A transparent cache for datastore, backed by memcache.

A transparent cache for datastore, backed by memcache.

Check doc.go for details, but:
  * allows cache disablement on a per-model/Key basis
  * allows cache expiration on a per-model basis
  * efficient (non-gob) encoding scheme
  * gzip for large entities
  * allows randomized sharding for Gets
    * allows good throughput for hot items
    * shards may be determined on a per-Key basis
  * caches negative hits

R=dnj@chromium.org, vadimsh@chromium.org, estaab@chromium.org, tandrii@chromium.org
BUG=

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

[iannucci, 2015-08-04 05:18:02]

On 2015/08/04 05:17:30, iannucci wrote:

Has ~65% coverage in tests, will be adding more, but it basically works and ppl
can start picking at it.

[iannucci, 2015-08-04 05:26:17]

https://codereview.chromium.org/1269113005/diff/1/filter/dscache/dscache_test.go
File filter/dscache/dscache_test.go (right):

https://codereview.chromium.org/1269113005/diff/1/filter/dscache/dscache_test...
filter/dscache/dscache_test.go:82:
[]byte("\x00\x80\x80W1[\x8fW(\x80\x80\x06i5@"))
for reference, the equivalent gob encoding is 57 bytes :P

[iannucci, 2015-08-05 08:00:41]

OK, PTAL. Tests only cover ~96%, but the rest is really boring crap (like
serialization IO error cases... how many ways can we make a buffer return EOF?).

I'm sure I'm missing some screw cases in the tests. If you have a favorite screw
case that you can think of, please let me know and I'll add a test for it (so we
can at least see what the expected behavior is).

[Vadim Sh., 2015-08-05 17:09:31]

(reviewed doc.go only)

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go
File filter/dscache/doc.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:26: //   - SHA1 has been chosen as unlikely (p == 1e-18)
to collide, given dedicated
just curious: why not use the key directly? Too long?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:30: //     cache (and also MD5 sucks).
that's a good argument :D

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:37: // The memcache entry may also have a 'flags' value
set to one of the following:
where 'flags' value is stored?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:43: // On a Put (or Delete), the memcache value written
with a LockTimeSeconds
unconditionally written? Overriding any existing stuff?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:48: // delete all of the memcache locks (Not using
CompareAndSwap).
what are "memcache locks"?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:56: // If it doesn't exist (unlikely since we just Add'd
it) or if it's flag is
typo: its (not it's)

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:68: // Algorithm - Transactions
What about Gets in transaction?

DS transactions use snapshot isolation (or whatever it is called).

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:89: //   - "dscache.enable,<true|false>" - whether or not
this entity should be
isn't it $dscache.enable?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:95: // returns the number of shards to use for a given
datastore key.
what does it mean to use sharding for a single datastore key?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:102: //   - It does NOT provide in-memory ("per-request")
caching.
good :)

[dnj, 2015-08-05 18:32:18]

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/context.go
File filter/dscache/context.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/context...
filter/dscache/context.go:39: logging.Get(ic),
Why not retain the context itself, instead of pulling out the logger? If you
retain the context, you can use logger fields (notably error).

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go
File filter/dscache/ds.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:42: d.log.Errorf("dscache: mc.AddMulti: %s", err)
(Debugf)

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:45: d.log.Errorf("dscache: mc.GetMulti: %s", err)
(Debugf)

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:50: if !p.empty() {
Invert: if p.empty() { return nil }

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:51: toCas := []memcache.Item{}
Might as well optimistically allocate capacity.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:75: if err != nil {
} else {

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
File filter/dscache/ds_txn_state.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
filter/dscache/ds_txn_state.go:34: func (s *dsTxnState) Apply(sc
*supportContext) {
Is there a reason these methods are exported? Maybe should be lowercase.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
filter/dscache/ds_txn_state.go:38: err := sc.mc.SetMulti(s.toLock)
IMO copy "toLock" while locked, but do the SetMulti outside of lock scope.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
filter/dscache/ds_txn_state.go:40: sc.log.Errorf("dscache: txn.Apply SetMulti:
%s", err)
Debugf?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
filter/dscache/ds_txn_state.go:53: err := sc.mc.DeleteMulti(delKeys)
IMO build "delKeys" while locked, but do DelMulti outside of lock scope.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/dscache.go
File filter/dscache/dscache.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/dscache...
filter/dscache/dscache.go:40: MemcacheVersion = "1"
Do all of these need to be exported?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go
File filter/dscache/plan.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go...
filter/dscache/plan.go:66: if flg == ItemHasLock {
Consider "switch" on "flag", clearer what values/actions you expect.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go...
filter/dscache/plan.go:67: if !bytes.Equal(f.nonce, lockItm.Value()) {
Invert this to make it more readable: if bytes.Equal(...) { p.add(...) }

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support.go
File filter/dscache/support.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:56: if len(ret) == 0 {
Is this necessary? Either it is allocated and gets populated, or it is still nil
from not being populated. If len(keys) == 0, the "for" will not be entered, so
no grow.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:85: // mocked randomness source, we use that.
Worth mentioning that we're using it for sharding, not crypto, and it should be
just fine for that purpose. IMO remove the TODO.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:108: s.log.Errorf("dscache: mc.SetMulti: %s", err)
This could be a lot of logging if memcache is not working. Since it's allowed to
fail, IMO make this "Debugf".

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:110: err := f()
if err := f(); err == nil {
...
}

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:113: s.log.Errorf("dscache: mc.DeleteMulti: %s", err)
(same)

https://codereview.chromium.org/1269113005/diff/150001/service/memcache/memca...
File service/memcache/memcache.go (right):

https://codereview.chromium.org/1269113005/diff/150001/service/memcache/memca...
service/memcache/memcache.go:36: func filterItems(lme *errors.LazyMultiError,
items []Item, nilErr error) ([]Item, []int) {
We can catch duplicate "set" here, too, by working backwards and discarding
duplicate keys.

https://codereview.chromium.org/1269113005/diff/150001/service/memcache/memca...
service/memcache/memcache.go:53: j := 0
What was wrong with "i"?

[Vadim Sh., 2015-08-06 01:23:34]

Ok, looked through most of it. My brain hurts. Consider making smaller code
dumps :( 

High level questions:
1) Is sharding worth the additional complexity? Do you really expect someone to
manually adjust number of shards?
2) I didn't understand what's going on with Gets in transaction. They should
directly hit datastore. Do they?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/context.go
File filter/dscache/context.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/context...
filter/dscache/context.go:20: // FilterRDS installs a counter RawDatastore
filter in the context.
not a counter

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go
File filter/dscache/ds.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:41: if err := d.mc.AddMulti(lockItems); err != nil {
what happens if only one item is failed to lock? (Is AddMulti atomic btw)?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:55: toSave := p.toSave[j]
ah, I guess appending nils is by design. Looked werid though. Consider using a
list of structs instead of two "parallel" arrays.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:55: toSave := p.toSave[j]
nit: move j++ to this line. I forgot what "j" was when saw it at the bottom.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:68: saveState = 2
heh... smart :)

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn.go
File filter/dscache/ds_txn.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn....
filter/dscache/ds_txn.go:16: sc *supportContext
so what happens with Gets in transaction? I'm lost among all context.Context
manipulations and can't figure out what's exactly called in transactions.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
File filter/dscache/ds_txn_state.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
filter/dscache/ds_txn_state.go:38: err := sc.mc.SetMulti(s.toLock)
It took me a while to figure out what's going on. Document that Apply is called
from inside transaction before commit and Release right after the commit.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/dscache.go
File filter/dscache/dscache.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/dscache...
filter/dscache/dscache.go:25: CacheTimeSeconds int64 = 0
I vote for "1 day" or something. To keep memcache clean and tidy. I don't think
1 day vs infinite makes much difference performance-wise.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/dscache...
filter/dscache/dscache.go:108: // of datastore keys?
why?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go
File filter/dscache/plan.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go...
filter/dscache/plan.go:39: p.toSave = append(p.toSave, save)
is it ok that you append 'nil's here?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go...
filter/dscache/plan.go:50: func makePlan(aid, ns string, log logging.Logger, f
*facts) *plan {
nit: makeFetchPlan or something

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go...
filter/dscache/plan.go:60: if lockItm == nil { // this item wasn't cacheable
typo: "wasn't cached" (unless I misunderstand something)

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/seriali...
File filter/dscache/serialize.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/seriali...
filter/dscache/serialize.go:27: writer := zlib.NewWriter(buf2)
does native SDK uses same trick? I thought compression is implemented in
appserver (behind RPC).

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support.go
File filter/dscache/support.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:54: ret[i] = MakeMemcacheKey(s.mr.Intn(shards), key)
so shards reduce contention on reads by making writes more expensive ('put' must
invalidate all shards)?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:83: // crappyNonce creates a really crappy nonce using
math/rand. This is generally
lol

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:90: // TODO(riannucci): make this use crypto/rand
instead
do you see any exploitation possibilities? Not all random must be secure random.
math/rand is good enough IMHO.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:127: if k == "" {
is it possible?

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:131: SetFlags(uint32(ItemHasLock)).
ah, I didn't know about flags. Cool.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:132: SetExpiration(time.Second *
time.Duration(LockTimeSeconds)).
why not define LockTimeSeconds to be of type time.Duration?

[iannucci, 2015-08-06 01:54:02]

PTAL again

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/context.go
File filter/dscache/context.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/context...
filter/dscache/context.go:39: logging.Get(ic),
On 2015/08/05 18:32:17, dnj wrote:
> Why not retain the context itself, instead of pulling out the logger? If you
> retain the context, you can use logger fields (notably error).

Mer... so... these are all used in inner loops. I didn't like the idea of
traversing the context linked list a whole bunch of times for each thingy. I
could maybe be convinced otherwise. Maybe :p

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go
File filter/dscache/doc.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:26: //   - SHA1 has been chosen as unlikely (p == 1e-18)
to collide, given dedicated
On 2015/08/05 17:09:31, Vadim Sh. wrote:
> just curious: why not use the key directly? Too long?

Yeah, there's a limit of 250 bytes for the memcache key (and it also may not
contain null bytes, so I can't just gzip some encoding of it). Additionally,
long keys eat into the available space for a memcache row (overhead + key +
value). I figured this was a pretty good compromise.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:37: // The memcache entry may also have a 'flags' value
set to one of the following:
On 2015/08/05 17:09:31, Vadim Sh. wrote:
> where 'flags' value is stored?

It's 4 of the bytes in the 'overhead' of the memcache implementation. It's
SetFlags(uint32) on memcache.Item.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:43: // On a Put (or Delete), the memcache value written
with a LockTimeSeconds
On 2015/08/05 17:09:31, Vadim Sh. wrote:
> unconditionally written? Overriding any existing stuff?

Yeah, it writes a lock (blank value + 30 second timeout) to exclude any Gets
from trying to populate the memcache entry while the put is doing it's thing.
Afterwards it's deleted. The 30s expiration is in the event of a crash.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:48: // delete all of the memcache locks (Not using
CompareAndSwap).
On 2015/08/05 17:09:31, Vadim Sh. wrote:
> what are "memcache locks"?

Hopefully clarified this section

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:56: // If it doesn't exist (unlikely since we just Add'd
it) or if it's flag is
On 2015/08/05 17:09:31, Vadim Sh. wrote:
> typo: its (not it's)

Done.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:68: // Algorithm - Transactions
On 2015/08/05 17:09:31, Vadim Sh. wrote:
> What about Gets in transaction?
> 
> DS transactions use snapshot isolation (or whatever it is called).

Gets pass through without any memcache interaction (reads or writes)

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:89: //   - "dscache.enable,<true|false>" - whether or not
this entity should be
On 2015/08/05 17:09:31, Vadim Sh. wrote:
> isn't it $dscache.enable?

clarified

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/doc.go#...
filter/dscache/doc.go:95: // returns the number of shards to use for a given
datastore key.
On 2015/08/05 17:09:31, Vadim Sh. wrote:
> what does it mean to use sharding for a single datastore key?

clarified

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go
File filter/dscache/ds.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:42: d.log.Errorf("dscache: mc.AddMulti: %s", err)
On 2015/08/05 18:32:17, dnj wrote:
> (Debugf)

Did warn

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:45: d.log.Errorf("dscache: mc.GetMulti: %s", err)
On 2015/08/05 18:32:17, dnj wrote:
> (Debugf)

Did warn

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:50: if !p.empty() {
On 2015/08/05 18:32:17, dnj wrote:
> Invert: if p.empty() { return nil }

can't, this modifies p.decoded for the loop below it.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:51: toCas := []memcache.Item{}
On 2015/08/05 18:32:17, dnj wrote:
> Might as well optimistically allocate capacity.

Eh... estimating the amount to allocate is hard enough and append does quadratic
allocation which should basically be fine.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:75: if err != nil {
On 2015/08/05 18:32:17, dnj wrote:
> } else {

Done.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
File filter/dscache/ds_txn_state.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
filter/dscache/ds_txn_state.go:34: func (s *dsTxnState) Apply(sc
*supportContext) {
On 2015/08/05 18:32:18, dnj wrote:
> Is there a reason these methods are exported? Maybe should be lowercase.

Done.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
filter/dscache/ds_txn_state.go:38: err := sc.mc.SetMulti(s.toLock)
On 2015/08/05 18:32:18, dnj wrote:
> IMO copy "toLock" while locked, but do the SetMulti outside of lock scope.

That could only happen if we're ending the transaction multiple times somehow :D

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
filter/dscache/ds_txn_state.go:40: sc.log.Errorf("dscache: txn.Apply SetMulti:
%s", err)
On 2015/08/05 18:32:17, dnj wrote:
> Debugf?

this ones a hard failure

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
filter/dscache/ds_txn_state.go:53: err := sc.mc.DeleteMulti(delKeys)
On 2015/08/05 18:32:18, dnj wrote:
> IMO build "delKeys" while locked, but do DelMulti outside of lock scope.

Also can only happen if we're ending the transaction multiple times

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/dscache.go
File filter/dscache/dscache.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/dscache...
filter/dscache/dscache.go:40: MemcacheVersion = "1"
On 2015/08/05 18:32:18, dnj wrote:
> Do all of these need to be exported?

no :D but I like them to be :P

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go
File filter/dscache/plan.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go...
filter/dscache/plan.go:66: if flg == ItemHasLock {
On 2015/08/05 18:32:18, dnj wrote:
> Consider "switch" on "flag", clearer what values/actions you expect.

Done.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go...
filter/dscache/plan.go:67: if !bytes.Equal(f.nonce, lockItm.Value()) {
On 2015/08/05 18:32:18, dnj wrote:
> Invert this to make it more readable: if bytes.Equal(...) { p.add(...) }

Done.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support.go
File filter/dscache/support.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:54: ret[i] = MakeMemcacheKey(s.mr.Intn(shards), key)
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> so shards reduce contention on reads by making writes more expensive ('put'
must
> invalidate all shards)?

Right

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:56: if len(ret) == 0 {
On 2015/08/05 18:32:18, dnj wrote:
> Is this necessary? Either it is allocated and gets populated, or it is still
nil
> from not being populated. If len(keys) == 0, the "for" will not be entered, so
> no grow.

o rite

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:85: // mocked randomness source, we use that.
On 2015/08/05 18:32:18, dnj wrote:
> Worth mentioning that we're using it for sharding, not crypto, and it should
be
> just fine for that purpose. IMO remove the TODO.

ya, did

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:90: // TODO(riannucci): make this use crypto/rand
instead
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> do you see any exploitation possibilities? Not all random must be secure
random.
> math/rand is good enough IMHO.

Yeah it's actually fine... just felt dirty extracting []byte from math/rand.
Update comment.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:108: s.log.Errorf("dscache: mc.SetMulti: %s", err)
On 2015/08/05 18:32:18, dnj wrote:
> This could be a lot of logging if memcache is not working. Since it's allowed
to
> fail, IMO make this "Debugf".

changed to a hard failz

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:110: err := f()
On 2015/08/05 18:32:18, dnj wrote:
> if err := f(); err == nil {
> ...
> }

Nah, I return it in the else case.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:113: s.log.Errorf("dscache: mc.DeleteMulti: %s", err)
On 2015/08/05 18:32:18, dnj wrote:
> (same)

a warning now

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:127: if k == "" {
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> is it possible?

Yeah. This is done if mkRandKeys detects that some of the keys have caching
disabled. I have tests that cover this, FWIW.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/support...
filter/dscache/support.go:132: SetExpiration(time.Second *
time.Duration(LockTimeSeconds)).
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> why not define LockTimeSeconds to be of type time.Duration?

Because SetExpiration truncates anything shorter than seconds. Didn't want to
give false hope if someone set it to a millisecond (and then memcache treats it
as infinite :D).

https://codereview.chromium.org/1269113005/diff/150001/service/memcache/memca...
File service/memcache/memcache.go (right):

https://codereview.chromium.org/1269113005/diff/150001/service/memcache/memca...
service/memcache/memcache.go:36: func filterItems(lme *errors.LazyMultiError,
items []Item, nilErr error) ([]Item, []int) {
On 2015/08/05 18:32:18, dnj wrote:
> We can catch duplicate "set" here, too, by working backwards and discarding
> duplicate keys.

uuuuggggggghhhhh :(

https://codereview.chromium.org/1269113005/diff/150001/service/memcache/memca...
service/memcache/memcache.go:53: j := 0
On 2015/08/05 18:32:18, dnj wrote:
> What was wrong with "i"?

b/c I'm using i for the index into the input map and j into the idxMap... not
apparent right here, I know, but I do it elsewhere.

https://codereview.chromium.org/1269113005/diff/150001/service/memcache/memca...
service/memcache/memcache.go:103: i := idxMap[j]
like right here!

[iannucci, 2015-08-06 02:37:33]

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/context.go
File filter/dscache/context.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/context...
filter/dscache/context.go:20: // FilterRDS installs a counter RawDatastore
filter in the context.
On 2015/08/06 01:23:33, Vadim Sh. wrote:
> not a counter

erp

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go
File filter/dscache/ds.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:41: if err := d.mc.AddMulti(lockItems); err != nil {
On 2015/08/06 01:23:33, Vadim Sh. wrote:
> what happens if only one item is failed to lock? (Is AddMulti atomic btw)?

it's not atomic, but it doesn't matter. Either:
  * it's being put, so we get from datastore and don't update the cache
  * someone already filled the value in and so Add is a noop (and then we just
decode from memcache)
  * someone else is locking, so we get from datastore and don't update the cache
  * memcache had a brain fart, so we... that thing ^
  * we did add it, but it still doesn't matter, since we need to re-get it in
order to compare and swap at the end of the function.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:68: saveState = 2
On 2015/08/06 01:23:33, Vadim Sh. wrote:
> heh... smart :)

managed to reduce this to just a boolean, but yeah, locking this one until the
next put is just an all around good idea.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn.go
File filter/dscache/ds_txn.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn....
filter/dscache/ds_txn.go:16: sc *supportContext
On 2015/08/06 01:23:33, Vadim Sh. wrote:
> so what happens with Gets in transaction? I'm lost among all context.Context
> manipulations and can't figure out what's exactly called in transactions.

documented, but all methods that aren't DeleteMulti or PutMulti just fly
straight through.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
File filter/dscache/ds_txn_state.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds_txn_...
filter/dscache/ds_txn_state.go:38: err := sc.mc.SetMulti(s.toLock)
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> It took me a while to figure out what's going on. Document that Apply is
called
> from inside transaction before commit and Release right after the commit.

Done.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/dscache.go
File filter/dscache/dscache.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/dscache...
filter/dscache/dscache.go:25: CacheTimeSeconds int64 = 0
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> I vote for "1 day" or something. To keep memcache clean and tidy. I don't
think
> 1 day vs infinite makes much difference performance-wise.

Sure. Done.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/dscache...
filter/dscache/dscache.go:108: // of datastore keys?
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> why?

Not sure... that's why a todo question :D

The thought was to possibly manually evict some, but not all, of the cache.
Though I think that just flushing the entire memcache is probably better/safer.

Removed

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go
File filter/dscache/plan.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go...
filter/dscache/plan.go:39: p.toSave = append(p.toSave, save)
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> is it ok that you append 'nil's here?

yeah. nil means "don't save it" I added a bunch of docs here to hopefully clear
things up.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go...
filter/dscache/plan.go:50: func makePlan(aid, ns string, log logging.Logger, f
*facts) *plan {
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> nit: makeFetchPlan or something

Done.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/plan.go...
filter/dscache/plan.go:60: if lockItm == nil { // this item wasn't cacheable
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> typo: "wasn't cached" (unless I misunderstand something)

fixed comment, it meant something different.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/seriali...
File filter/dscache/serialize.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/seriali...
filter/dscache/serialize.go:27: writer := zlib.NewWriter(buf2)
On 2015/08/06 01:23:34, Vadim Sh. wrote:
> does native SDK uses same trick? I thought compression is implemented in
> appserver (behind RPC).

This is for compressing the values inside of memcache, unless I'm not
understanding what you're meaning?

[iannucci, 2015-08-07 00:05:18]

PTAL/pang :)

[dnj, 2015-08-07 16:30:07]

lgtm w/ nits

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go
File filter/dscache/ds.go (right):

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:50: if !p.empty() {
On 2015/08/06 01:54:01, iannucci wrote:
> On 2015/08/05 18:32:17, dnj wrote:
> > Invert: if p.empty() { return nil }
> 
> can't, this modifies p.decoded for the loop below it.

Acknowledged.

https://codereview.chromium.org/1269113005/diff/150001/filter/dscache/ds.go#n...
filter/dscache/ds.go:51: toCas := []memcache.Item{}
On 2015/08/06 01:54:01, iannucci wrote:
> On 2015/08/05 18:32:17, dnj wrote:
> > Might as well optimistically allocate capacity.
> 
> Eh... estimating the amount to allocate is hard enough and append does
quadratic
> allocation which should basically be fine.

Acknowledged.

https://codereview.chromium.org/1269113005/diff/250001/filter/dscache/dscache.go
File filter/dscache/dscache.go (right):

https://codereview.chromium.org/1269113005/diff/250001/filter/dscache/dscache...
filter/dscache/dscache.go:36: CacheTimeSeconds int64 = int64((time.Hour *
24).Seconds())
WDYT about just making this a "time.Duration" w/ the comment note that the value
will be rounded up to seconds? No need to fudge around with integer values when
discussing time intervals.

https://codereview.chromium.org/1269113005/diff/250001/filter/dscache/dscache...
filter/dscache/dscache.go:122: type GlobalConfig struct {
IMO move to a separate ".go" file.

https://codereview.chromium.org/1269113005/diff/250001/filter/dscache/dscache...
filter/dscache/dscache.go:157: globalEnabledLock.RLock()
For peace of mind (and to avoid C-style cleanup patterns), consider one of:
globalEnabledLock.RLock()
nextCheck := globalEnabledNextCheck
globalEnabledLock.RUnlock()
if now.Before(nextCheck) { ... }

-or-

def getGlobalEnabledNextCheck() time.Time {
  globalEnabledLock.RLock()
  defer globalEnabledLock.RUnlock()
  return globalEnabled
}

...

if now.Before(getGlobalEnabledNextCheck()) { ... }

https://codereview.chromium.org/1269113005/diff/250001/filter/dscache/dscache...
filter/dscache/dscache.go:171: // alawys go to the default namespace
always

https://codereview.chromium.org/1269113005/diff/250001/filter/dscache/dscache...
filter/dscache/dscache.go:174: return true
When the memcache failure case can perpetuate stale data, perhaps the default
error result should be "false".

https://codereview.chromium.org/1269113005/diff/250001/filter/dscache/dscache...
filter/dscache/dscache.go:182: return globalEnabled
Just return "cfg.Enable".

https://codereview.chromium.org/1269113005/diff/250001/filter/dscache/dscache...
filter/dscache/dscache.go:185: func SetDynamicGlobalEnable(c context.Context,
memcacheEnabled bool) error {
Why "Dynamic"? Seems inconsistent with the other variables/methods.

[Vadim Sh., 2015-08-07 17:30:47]

lgtm with nit

also add a TODO to investigate a "fast path" algorithm to do one roundtrip to
memcache for cached items (one Get) instead of two (Add\Get).

https://codereview.chromium.org/1269113005/diff/250001/filter/dscache/doc.go
File filter/dscache/doc.go (right):

https://codereview.chromium.org/1269113005/diff/250001/filter/dscache/doc.go#...
filter/dscache/doc.go:116: // at random. This is good for heavily-read, but
infrequently updated, entities.
nit: mention that it's needed to avoid having hot keys in memcached, e.g. the
contention happens in memcached guts, not in our code. You sort of mention this
below, but it would be clearer here.

[iannucci, 2015-08-07 19:40:36]

Committed patchset #15 (id:260001) manually as
bf5c5a93f90884d7f1a85ad5ad68e5b3a323e6f9 (presubmit successful).

[iannucci, 2015-08-07 19:41:00]

oh, fixed all the nits btw

https://chromiumcodereview.appspot.com/1269113005/diff/250001/filter/dscache/...
File filter/dscache/doc.go (right):

https://chromiumcodereview.appspot.com/1269113005/diff/250001/filter/dscache/...
filter/dscache/doc.go:116: // at random. This is good for heavily-read, but
infrequently updated, entities.
On 2015/08/07 17:30:47, Vadim Sh. wrote:
> nit: mention that it's needed to avoid having hot keys in memcached, e.g. the
> contention happens in memcached guts, not in our code. You sort of mention
this
> below, but it would be clearer here.

Done.

https://chromiumcodereview.appspot.com/1269113005/diff/250001/filter/dscache/...
File filter/dscache/dscache.go (right):

https://chromiumcodereview.appspot.com/1269113005/diff/250001/filter/dscache/...
filter/dscache/dscache.go:36: CacheTimeSeconds int64 = int64((time.Hour *
24).Seconds())
On 2015/08/07 16:30:06, dnj wrote:
> WDYT about just making this a "time.Duration" w/ the comment note that the
value
> will be rounded up to seconds? No need to fudge around with integer values
when
> discussing time intervals.

I mentioned this elsewhere, but memcache truncates to the second. This means if
you set this to, say, less than a second, it will actually be infinite. I
updated this comment.

https://chromiumcodereview.appspot.com/1269113005/diff/250001/filter/dscache/...
filter/dscache/dscache.go:122: type GlobalConfig struct {
On 2015/08/07 16:30:06, dnj wrote:
> IMO move to a separate ".go" file.

Done.

https://chromiumcodereview.appspot.com/1269113005/diff/250001/filter/dscache/...
filter/dscache/dscache.go:157: globalEnabledLock.RLock()
On 2015/08/07 16:30:07, dnj wrote:
> For peace of mind (and to avoid C-style cleanup patterns), consider one of:
> globalEnabledLock.RLock()
> nextCheck := globalEnabledNextCheck
> globalEnabledLock.RUnlock()
> if now.Before(nextCheck) { ... }

derp, dunno what I was thinking. did this.

> 
> -or-
> 
> def getGlobalEnabledNextCheck() time.Time {
>   globalEnabledLock.RLock()
>   defer globalEnabledLock.RUnlock()
>   return globalEnabled
> }
> 
> ...
> 
> if now.Before(getGlobalEnabledNextCheck()) { ... }

https://chromiumcodereview.appspot.com/1269113005/diff/250001/filter/dscache/...
filter/dscache/dscache.go:171: // alawys go to the default namespace
On 2015/08/07 16:30:06, dnj wrote:
> always

Done.

https://chromiumcodereview.appspot.com/1269113005/diff/250001/filter/dscache/...
filter/dscache/dscache.go:174: return true
On 2015/08/07 16:30:06, dnj wrote:
> When the memcache failure case can perpetuate stale data, perhaps the default
> error result should be "false".

Nope, because then you could end up doing Put operations without invalidating
the memcache, where all the other instances would then read stale data. I
explain this in the comment on this function :)

https://chromiumcodereview.appspot.com/1269113005/diff/250001/filter/dscache/...
filter/dscache/dscache.go:182: return globalEnabled
On 2015/08/07 16:30:06, dnj wrote:
> Just return "cfg.Enable".

I actually prefer not to. globalEnabled is the value that everything in the
program sees, but cfg.Enable is local to this function. if future code ends up
not updating gloablEnabled for some reason, then this will continue to return a
program-wide consistent value. If I make it cfg.Enable, it could return an
inconsistent value.

https://chromiumcodereview.appspot.com/1269113005/diff/250001/filter/dscache/...
filter/dscache/dscache.go:185: func SetDynamicGlobalEnable(c context.Context,
memcacheEnabled bool) error {
On 2015/08/07 16:30:07, dnj wrote:
> Why "Dynamic"? Seems inconsistent with the other variables/methods.

old naming artifact. fixed.