impl/cloud: Add support for "memcached" memcache.

impl/cloud: Add support for "memcached" memcache.

Add "memcache" service support to "impl/cloud" backed by a "memcached"
cluster.

This enables a full dscache-backed datastore to be implemneted using
"impl/cloud", as well as other services that need memcache to operate.

Update the "impl/cloud" interface to start with a service Config and
install services from that.

BUG=None
TEST=local
  - Started "memcached" server, tested against it.

Committed: https://github.com/luci/gae/commit/a9451708f36e28488bea90aab94ce84153ead26f

[dnj, 2016-11-19 18:29:38]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org

[dnj, 2016-11-19 18:29:39]

PTAL

[iannucci, 2016-11-20 19:13:59]

typo in commit message

[iannucci, 2016-11-20 19:35:17]

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/context.go
File impl/cloud/context.go (right):

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/context.go#n...
impl/cloud/context.go:28: type Config struct {
This feels kinda wrong; there probably should be a 'setup' package which
contains all the various configurations, and then the impl/* should have
individual methods to get the various implementations or something. Then the
setup package could contain all the standard configurations (e.g. running in GAE
classic/Cloud/Memory, etc.) and any configurable hybrids that make sense.

Don't need to fix in this CL, but something to think about. Basically the
'service' packages already allow you to set the implementations independently.
Up till now there hasn't been any need for configuring mix and match
implementations, but now that there is I don't think it makes sense for that
configuration library code to live inside of one (or many) impl packages, if
that makes sense.

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go
File impl/cloud/memcache.go (right):

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go#...
impl/cloud/memcache.go:29: // entries by prepending
"memcacheKeyPrefix:SHA256(namespace):" to each key.
note that this is basically how it's done in prod. It's pretty common afaik. Try
to match behaviors documented here:
https://cloud.google.com/appengine/docs/java/memcache/#limits

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go#...
impl/cloud/memcache.go:196: // The Memcache service doesn't have an
"IncrementExisting" equivalent. We
Should we move this up to a top-level function? I think all the memcache
implementations have compare-and-swap, which would be enough to do this in
service/memcache instead of in every individual implementation.

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go#...
impl/cloud/memcache.go:251: // namespace, so Flush will flush all memcache.
I'm pretty sure this is the same behavior in prod...

[dnj, 2016-11-20 22:28:47]

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/context.go
File impl/cloud/context.go (right):

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/context.go#n...
impl/cloud/context.go:28: type Config struct {
On 2016/11/20 19:35:16, iannucci wrote:
> This feels kinda wrong; there probably should be a 'setup' package which
> contains all the various configurations, and then the impl/* should have
> individual methods to get the various implementations or something. Then the
> setup package could contain all the standard configurations (e.g. running in
GAE
> classic/Cloud/Memory, etc.) and any configurable hybrids that make sense.
> 
> Don't need to fix in this CL, but something to think about. Basically the
> 'service' packages already allow you to set the implementations independently.
> Up till now there hasn't been any need for configuring mix and match
> implementations, but now that there is I don't think it makes sense for that
> configuration library code to live inside of one (or many) impl packages, if
> that makes sense.

Yeah the mix-and-match world is definitely weird. I think for now having a
homogenous "cloud services" vision is fine. Splitting it up into pick-and-choose
can get more creative, but I think will still be containable.

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go
File impl/cloud/memcache.go (right):

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go#...
impl/cloud/memcache.go:29: // entries by prepending
"memcacheKeyPrefix:SHA256(namespace):" to each key.
On 2016/11/20 19:35:16, iannucci wrote:
> note that this is basically how it's done in prod. It's pretty common afaik.
Try
> to match behaviors documented here:
> https://cloud.google.com/appengine/docs/java/memcache/#limits

Oh cool, I'll add null-byte restriction and >250 byte key hashing.

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go#...
impl/cloud/memcache.go:196: // The Memcache service doesn't have an
"IncrementExisting" equivalent. We
On 2016/11/20 19:35:16, iannucci wrote:
> Should we move this up to a top-level function? I think all the memcache
> implementations have compare-and-swap, which would be enough to do this in
> service/memcache instead of in every individual implementation.

TBH I think the implementation here is pretty optimal. Expected 1 API call on
exists, 2 on doesn't-exist, 4 if cache is unfortunately flushed in between.

Since GAE offers this as an API option, IMO we should continue to use it. GAE
internally supports both Increment and IncrementExisting at the protobuf level
(https://github.com/golang/appengine/blob/master/memcache/memcache.go), so I
think this is fine to leave at impl-level. The logic here isn't too bad and can
easily be elevated if we see an actual need in the future.

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go#...
impl/cloud/memcache.go:251: // namespace, so Flush will flush all memcache.
On 2016/11/20 19:35:16, iannucci wrote:
> I'm pretty sure this is the same behavior in prod...

Hopefully prod at least flushes per-customer. I envision this as a memcached
situation with N other random users and us. Us might have M namespaces, and I'm
okay flushing M, but flushing the other N users seems unfortunate. Hopefully GAE
can manage to only flush the N.

[dnj, 2016-11-21 05:24:33]

PTAL. I added the key hashing threshold, rest is the same.

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go
File impl/cloud/memcache.go (right):

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go#...
impl/cloud/memcache.go:29: // entries by prepending
"memcacheKeyPrefix:SHA256(namespace):" to each key.
On 2016/11/20 22:28:47, dnj wrote:
> On 2016/11/20 19:35:16, iannucci wrote:
> > note that this is basically how it's done in prod. It's pretty common afaik.
> Try
> > to match behaviors documented here:
> > https://cloud.google.com/appengine/docs/java/memcache/#limits
> 
> Oh cool, I'll add null-byte restriction and >250 byte key hashing.

Actually on second thought:

The null-byte restriction makes no sense at the impl/cloud level given the
current implementation, so I don't feel the need to enforce it. The memcache key
length seems good.

If we want to enforce things like the null-byte restriction, we should add a
checkfilter to the memcache service so it is homogeneously asserted.

[dnj, 2016-11-23 16:45:01]

(friendly ping)

[iannucci, 2016-11-23 21:14:37]

lgtm

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go
File impl/cloud/memcache.go (right):

https://codereview.chromium.org/2513253002/diff/20001/impl/cloud/memcache.go#...
impl/cloud/memcache.go:251: // namespace, so Flush will flush all memcache.
On 2016/11/20 22:28:47, dnj wrote:
> On 2016/11/20 19:35:16, iannucci wrote:
> > I'm pretty sure this is the same behavior in prod...
> 
> Hopefully prod at least flushes per-customer. I envision this as a memcached
> situation with N other random users and us. Us might have M namespaces, and
I'm
> okay flushing M, but flushing the other N users seems unfortunate. Hopefully
GAE
> can manage to only flush the N.

GAE flushes all of an app's keys (but all gae-namespaces used by that app). I
don't know that I've ever seen a memcached deployment where multiple
non-coordinating users were using the same one. That would lead to random
evictions when one of the other users decided to dump a crapload of stuff into
'their' memcache. If there are non-coordinating customers, they would always
have different memcached instances.

[dnj, 2016-11-24 01:04:38]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-11-24 01:04:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 01:09:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 01:09:58]

Try jobs failed on following builders:
  Luci-GAE Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/32acb12e8952fa10)

[dnj, 2016-11-24 05:21:41]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-11-24 05:21:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 05:25:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479964901692170,
"parent_rev": "3d3367c3e63be9d05b0df0a8c728575f785b21b0", "commit_rev":
"a9451708f36e28488bea90aab94ce84153ead26f"}

[commit-bot: I haz the power, 2016-11-24 05:25:29]

Description was changed from

==========
impl/cloud: Add support for "memcached" memcache.

Add "memcache" service support to "impl/cloud" backed by a "memcached"
cluster.

This enables a full dscache-backed datastore to be implemneted using
"impl/cloud", as well as other services that need memcache to operate.

Update the "impl/cloud" interface to start with a service Config and
install services from that.

BUG=None
TEST=local
  - Started "memcached" server, tested against it.
==========

to

==========
impl/cloud: Add support for "memcached" memcache.

Add "memcache" service support to "impl/cloud" backed by a "memcached"
cluster.

This enables a full dscache-backed datastore to be implemneted using
"impl/cloud", as well as other services that need memcache to operate.

Update the "impl/cloud" interface to start with a service Config and
install services from that.

BUG=None
TEST=local
  - Started "memcached" server, tested against it.

Committed:
https://github.com/luci/gae/commit/a9451708f36e28488bea90aab94ce84153ead26f
==========

[commit-bot: I haz the power, 2016-11-24 05:25:30]

Committed patchset #4 (id:60001) as
https://github.com/luci/gae/commit/a9451708f36e28488bea90aab94ce84153ead26f