Add transaction buffer filter.

Add transaction buffer filter.

This will be used for running multiple non-cooperative idempotent tasks inside
of tumble, which require seeing the previous tasks' effects on the entity group.

However, the effect of having transactions transparently buffer datastore interactions until the final transaction commit is a generally useful alteration of the current behavior.

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

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

[iannucci, 2015-08-25 03:46:08]

On 2015/08/25 03:45:50, iannucci wrote:

Tests will come soonly.

[iannucci, 2015-09-25 18:17:35]

OK, have added some tests and this actually works now. I don't expect to change
the code much more in this CL (just adding tests and fixing bugs they find).

[Vadim Sh., 2015-09-28 18:52:56]

haven't looked at query merging yet

this all looks vey scary...

https://codereview.chromium.org/1309803004/diff/80001/filter/txnBuf/context.go
File filter/txnBuf/context.go (right):

https://codereview.chromium.org/1309803004/diff/80001/filter/txnBuf/context.g...
filter/txnBuf/context.go:20: // FilterRDS installs a counter datastore filter in
the context.
counter?

https://codereview.chromium.org/1309803004/diff/80001/filter/txnBuf/ds.go
File filter/txnBuf/ds.go (right):

https://codereview.chromium.org/1309803004/diff/80001/filter/txnBuf/ds.go#new...
filter/txnBuf/ds.go:14: const transactionSizeLimit = 10 * 1000 * 1000
*1024*1024? :)

https://codereview.chromium.org/1309803004/diff/80001/impl/memory/datastore_i...
File impl/memory/datastore_index_selection.go (right):

https://codereview.chromium.org/1309803004/diff/80001/impl/memory/datastore_i...
impl/memory/datastore_index_selection.go:393: def.start =
def.start[offset+len(chopped):]
diffs like this look scary.. Was there a bug here?

https://codereview.chromium.org/1309803004/diff/80001/service/datastore/key.go
File service/datastore/key.go (right):

https://codereview.chromium.org/1309803004/diff/80001/service/datastore/key.g...
service/datastore/key.go:362: // HasAncestor returns true iff other is an
ancestor of k.
... or k is other.

https://codereview.chromium.org/1309803004/diff/80001/service/datastore/query.go
File service/datastore/query.go (right):

https://codereview.chromium.org/1309803004/diff/80001/service/datastore/query...
service/datastore/query.go:475: if hasAncestor {
nit:
ancestor := (*Key)(nil)
if slice, ok := q.eqFilts["__ancestor__"]; ok {
  ancestor = slice[0].Value().(*Key)
}

and then use ancestor != nil instead of hasAncestor.

(Unless nil ancestor is acceptable value..)

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/context.go
File filter/txnBuf/context.go (right):

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/context....
filter/txnBuf/context.go:24: return ds.AddRawFilters(c, func(c context.Context,
RawDatastore ds.RawInterface) ds.RawInterface {
rawDatastore

I was very confused trying to figure out where "exported" RawDatastore is
defined in &dsBuf{RawDatastore, info.Get(c).GetNamespace()}

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.go
File filter/txnBuf/ds_txn.go (right):

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:57: j++
this should probably be before err == ds.ErrNoSuchEntity check?

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:74: return nil
is it ok to return nil here even if lme contains some errors?

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:82: start, err := d.state.parentDS.AllocateIDs(key, 1)
in theory it should be possible to batch\parallelize this... Not sure it's worth
it, depends on use case.

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:108: return err
is it ok not to call callback here?

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:118: err := d.state.deleteMulti(keys)
what about deleting them from _real_ datastore? state.deleteMulti only deletes
from in-memory one as far as I can tell

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/state.go
File filter/txnBuf/state.go (right):

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/state.go...
filter/txnBuf/state.go:137: {
Remove...

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/state.go...
filter/txnBuf/state.go:242: impossible(err)
nit: move it up one line

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/state.go...
filter/txnBuf/state.go:312: fq, err := datastore.NewQuery("").Finalize()
wat?

[iannucci, 2015-09-29 03:21:38]

PTAnL

I'm going to go through and add authors notes as well.

https://codereview.chromium.org/1309803004/diff/80001/filter/txnBuf/context.go
File filter/txnBuf/context.go (right):

https://codereview.chromium.org/1309803004/diff/80001/filter/txnBuf/context.g...
filter/txnBuf/context.go:20: // FilterRDS installs a counter datastore filter in
the context.
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> counter?

copy pasta

https://codereview.chromium.org/1309803004/diff/80001/filter/txnBuf/ds.go
File filter/txnBuf/ds.go (right):

https://codereview.chromium.org/1309803004/diff/80001/filter/txnBuf/ds.go#new...
filter/txnBuf/ds.go:14: const transactionSizeLimit = 10 * 1000 * 1000
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> *1024*1024? :)

I was playing it safe. Not sure if they mean 1024 or 1000.

https://codereview.chromium.org/1309803004/diff/80001/impl/memory/datastore_i...
File impl/memory/datastore_index_selection.go (right):

https://codereview.chromium.org/1309803004/diff/80001/impl/memory/datastore_i...
impl/memory/datastore_index_selection.go:393: def.start =
def.start[offset+len(chopped):]
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> diffs like this look scary.. Was there a bug here?

Yeah there was. I fixed the tests for this too. Basically if you had an
inequality on __key__ and also had an ancestor and also ended up selecting a
built-in index, then this would incorrectly set the start/end points.

I actually had tests for these, but the expectations in the tests were totally
wrong >_<.

Now that you pointed it out, however, I was able to delete some more code here
which was an impossible case (the if statement above would only happen if you
had an inequality and an ancestor and a built-in index... but that's impossible!
if you look at the index listing on real appengine
(https://cloud.google.com/appengine/docs/go/datastore/indexes#Go_Index_configu...),
you can see that "Queries with ancestor and inequality filters" require new
indexes).

I also added some clarifying comment to this section.

https://codereview.chromium.org/1309803004/diff/80001/service/datastore/key.go
File service/datastore/key.go (right):

https://codereview.chromium.org/1309803004/diff/80001/service/datastore/key.g...
service/datastore/key.go:362: // HasAncestor returns true iff other is an
ancestor of k.
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> ... or k is other.

Yes. Done.

https://codereview.chromium.org/1309803004/diff/80001/service/datastore/query.go
File service/datastore/query.go (right):

https://codereview.chromium.org/1309803004/diff/80001/service/datastore/query...
service/datastore/query.go:475: if hasAncestor {
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> nit:
> ancestor := (*Key)(nil)
> if slice, ok := q.eqFilts["__ancestor__"]; ok {
>   ancestor = slice[0].Value().(*Key)
> }
> 
> and then use ancestor != nil instead of hasAncestor.
> 
> (Unless nil ancestor is acceptable value..)

Nope, you're right. nil ancestor means no ancestor. Fixed.

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/context.go
File filter/txnBuf/context.go (right):

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/context....
filter/txnBuf/context.go:24: return ds.AddRawFilters(c, func(c context.Context,
RawDatastore ds.RawInterface) ds.RawInterface {
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> rawDatastore
> 
> I was very confused trying to figure out where "exported" RawDatastore is
defined in &dsBuf{RawDatastore, info.Get(c).GetNamespace()}

derp. Done.

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.go
File filter/txnBuf/ds_txn.go (right):

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:57: j++
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> this should probably be before err == ds.ErrNoSuchEntity check?

ah, yeah. I added a test for this too.

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:74: return nil
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> is it ok to return nil here even if lme contains some errors?

yeah, that's the convention. Basically either the overall function returns an
error (RPC failure, argument error, etc.) or each callback gets an error (rpc
success, some things are missing, bad serialization on a per-item basis, etc.).

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:82: start, err := d.state.parentDS.AllocateIDs(key, 1)
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> in theory it should be possible to batch\parallelize this... Not sure it's
worth it, depends on use case.

Thought of this, and yeah it does. However I'm pretty sure that this is a very
cheap operation and that the RPC terminator already batches these. We can
profile and optimize if it's an issue (e.g. always batch by 10 or something).

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:108: return err
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> is it ok not to call callback here?

yeah per the convention.

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:118: err := d.state.deleteMulti(keys)
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> what about deleting them from _real_ datastore? state.deleteMulti only deletes
from in-memory one as far as I can tell

all _real_ datastore activity happens in the apply() method on the state object.
It's {0,1} DeleteMultis and {0,1} PutMultis, and they're dispatched in parallel.

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/state.go
File filter/txnBuf/state.go (right):

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/state.go...
filter/txnBuf/state.go:137: {
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> Remove...

k

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/state.go...
filter/txnBuf/state.go:242: impossible(err)
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> nit: move it up one line

done

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/state.go...
filter/txnBuf/state.go:312: fq, err := datastore.NewQuery("").Finalize()
On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> wat?

added comment

https://codereview.chromium.org/1309803004/diff/120001/impl/memory/datastore_...
File impl/memory/datastore_data.go (left):

https://codereview.chromium.org/1309803004/diff/120001/impl/memory/datastore_...
impl/memory/datastore_data.go:29: consistent bool
This change means that we avoid taking a snapshot on every mutation when we're
in full-consistency mode. This saves a couple .1s on the tests.

[Vadim Sh., 2015-09-29 03:27:26]

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.go
File filter/txnBuf/ds_txn.go (right):

https://codereview.chromium.org/1309803004/diff/120001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:118: err := d.state.deleteMulti(keys)
On 2015/09/29 03:21:38, iannucci wrote:
> On 2015/09/28 at 18:52:56, Vadim Sh. wrote:
> > what about deleting them from _real_ datastore? state.deleteMulti only
deletes
> from in-memory one as far as I can tell
> 
> all _real_ datastore activity happens in the apply() method on the state
object.
> It's {0,1} DeleteMultis and {0,1} PutMultis, and they're dispatched in
parallel.

Yes, I understand that. But if I open a new buffered transaction, do DeleteMulti
(and nothing else), and close it, will it actually go to the real datastore and
delete entites? From looking at the code, I seems it will try to delete entities
from (empty) memory buffer and then do nothing. (PS: I didn't read the tests,
and probably misunderstand 'state' implementation).

[iannucci, 2015-09-29 03:32:23]

https://codereview.chromium.org/1309803004/diff/160001/filter/txnBuf/doc.go
File filter/txnBuf/doc.go (right):

https://codereview.chromium.org/1309803004/diff/160001/filter/txnBuf/doc.go#n...
filter/txnBuf/doc.go:65: //     mentioning.
This doc is worth reading if you haven't. The limitations should be pretty
bearable, but just so you know :)

https://codereview.chromium.org/1309803004/diff/160001/filter/txnBuf/ds.go
File filter/txnBuf/ds.go (right):

https://codereview.chromium.org/1309803004/diff/160001/filter/txnBuf/ds.go#ne...
filter/txnBuf/ds.go:24: func (d *dsBuf) RunInTransaction(f func(context.Context)
error, opts *ds.TransactionOptions) error {
So you can see here that in not-a-transaction, literally all methods pass
straight through without any interference (except for this one).

https://codereview.chromium.org/1309803004/diff/160001/filter/txnBuf/state.go
File filter/txnBuf/state.go (right):

https://codereview.chromium.org/1309803004/diff/160001/filter/txnBuf/state.go...
filter/txnBuf/state.go:337: ds := t.parentDS
parentDS is either the real datastore transaction context, or one of the buffers
(if you're nested)

https://codereview.chromium.org/1309803004/diff/160001/filter/txnBuf/state.go...
filter/txnBuf/state.go:358: dErr = ds.DeleteMulti(toDel, func(err error) {
This is where the actual deletion occurs in the parent context. If parent is the
real datastore, then this is a real delete.

[iannucci, 2015-09-29 04:43:28]

added a bunch of comments (in code and in review). ptal

https://codereview.chromium.org/1309803004/diff/180001/filter/txnBuf/context.go
File filter/txnBuf/context.go (right):

https://codereview.chromium.org/1309803004/diff/180001/filter/txnBuf/context....
filter/txnBuf/context.go:26: return &dsTxnBuf{c, par}
So you can see here that while in a transaction, we /completely ignore/ the rds
that gets passed in (which is the previously filtered datastore object). So if
you have your filters like:

  DATASTORE -> A -> txnBuf -> B

In non-transactions, you'll get

  B(dsBuf(A(DATASTORE)))

And while in a transaction, you'll get:
  
  B(dsTxnBuf(parentState))

Where parentState is a linked list of states which eventually point back to the
real DATASTORE. Each state in the chain is a nested transaction.

https://codereview.chromium.org/1309803004/diff/180001/filter/txnBuf/ds_txn.go
File filter/txnBuf/ds_txn.go (right):

https://codereview.chromium.org/1309803004/diff/180001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:18: type dsTxnBuf struct {
previously this had a ds.RawInterface embedded in it. That masked the fact that
I was implementing Count wrong :(

https://codereview.chromium.org/1309803004/diff/180001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:134: func (d *dsTxnBuf) Count(fq *ds.FinalizedQuery)
(count int64, err error) {
Added Count since it was missing. Unfortunately we have to do this the dumb way
because running Count on the underlying stores won't give us enough information
to merge :/

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/context.go
File impl/memory/context.go (right):

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/context.go...
impl/memory/context.go:112: func UseWithAppID(c context.Context, aid string)
context.Context {
This was so that the in-memory buffered transactions can correctly inflate keys
depending on what the actual AppID is.

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/datastore_...
File impl/memory/datastore_index_selection.go (left):

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/datastore_...
impl/memory/datastore_index_selection.go:383: }
So it turned out that these were just wrong. These assumed that it was possible
to have an inequality AND an ancestor AND fill it with a builtin query. This is
not a possible combination, and there's now a check in maybeAddDefinition above
that will prevent this.

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/datastore_...
File impl/memory/datastore_query.go (right):

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/datastore_...
impl/memory/datastore_query.go:111: func GetBinaryBounds(fq *ds.FinalizedQuery)
(lower, upper []byte) {
This was factored out to allow toComparableString in txnBuf to use the same
logic.

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/datastore_...
File impl/memory/datastore_query_execution.go (right):

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/datastore_...
impl/memory/datastore_query_execution.go:147: if count >= 0 && i >= count {
This was another bug, added a test for it.

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/gkvlite_it...
File impl/memory/gkvlite_iter.go (right):

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/gkvlite_it...
impl/memory/gkvlite_iter.go:69: it.next(serialize.Join(def.prefix[:pfxLen],
suffix), func(itm *gkvlite.Item) {
This bug happens when using builtin indexes for queries with inequalities on
__key__ and also ancestor filters AND where the multiIterator query needs to
skip some rows. It happens because the bytes for the ancestor key don't reflect
in pfxLen (on purpose, see datastore_index_selection). I was correctly creating
sfxRO below, but failed to do the same here.

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/info.go
File impl/memory/info.go (left):

https://codereview.chromium.org/1309803004/diff/180001/impl/memory/info.go#ol...
impl/memory/info.go:39: const globalAppID = "dev~app"
routed through the internal apis :)

https://codereview.chromium.org/1309803004/diff/180001/service/datastore/fina...
File service/datastore/finalized_query.go (right):

https://codereview.chromium.org/1309803004/diff/180001/service/datastore/fina...
service/datastore/finalized_query.go:244: ws(" __key__")
so KeysOnly queries show up distinct from regular queries.

https://codereview.chromium.org/1309803004/diff/180001/service/datastore/inde...
File service/datastore/index.go (right):

https://codereview.chromium.org/1309803004/diff/180001/service/datastore/inde...
service/datastore/index.go:220: if id.Kind == "" || id.Builtin() {
duplicate logic is duplicate.

https://codereview.chromium.org/1309803004/diff/180001/service/datastore/quer...
File service/datastore/query.go (right):

https://codereview.chromium.org/1309803004/diff/180001/service/datastore/quer...
service/datastore/query.go:488: }
ancestor filters are technically equality filters here, but they don't count for
this restriction.

https://codereview.chromium.org/1309803004/diff/180001/service/datastore/quer...
service/datastore/query.go:584: seenOrders := stringset.New(len(q.order))
was getting duplicated orders because the if statement below wasn't adding the
inequality filter property to the seen orders.

[iannucci, 2015-09-29 20:08:43]

OK, refactored all the unrelated stuff out into separate CLs.

[Vadim Sh., 2015-09-30 01:11:29]

can you add simulated retries to some transactions in tests to verify it work
under such conditions?

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/ds_txn.go
File filter/txnBuf/ds_txn.go (right):

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:58: data[i].key = keys[j]
are you sure it shoud be 'j' here and not 'i'? Or alternatively use getKeys
instead of keys.

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:102: cb(nil, e)
what does cb(nil, nil) mean for Put? It's how it's called for "complete" keys if
AllocateIDs failed.

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:138: if len(fq.Project()) == 0 && !fq.KeysOnly() {
what if fq uses 'limit' or 'skip' or cursor? Worth adding a check.

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:161: needSlimming := len(project) > 0
nit: just use len(project) > 0 inline. It is using in a single place only.

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/query_me...
File filter/txnBuf/query_merger.go (right):

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/query_me...
filter/txnBuf/query_merger.go:40: if dedup != nil && dedup(encKey) {
why have both dedup & filter if they are used identically? Let caller do "return
dedupped() || filtered()" instead.

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/query_me...
filter/txnBuf/query_merger.go:62: if itm.err != nil {
not sure it matters, but iteration function returns (nil, err) only once,
returning (nil, nil) on next invocations. (i.e. error state is not "sticky").

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/state.go
File filter/txnBuf/state.go (right):

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/state.go...
filter/txnBuf/state.go:55: if s.keyToSize == nil {
err... you should probably swap this 'if' with previous line. It will look less
confusing.

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/state.go...
filter/txnBuf/state.go:114: siblingLock sync.Mutex
hm....

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/state.go...
filter/txnBuf/state.go:130: defer parentState.siblingLock.Unlock()
this is released at the very end of this function, including after cb is
finished.

Is it what you've intended?

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/state.go...
filter/txnBuf/state.go:258: impossible(err)
I have a feeling someday we will start seeing this impossible errors showing up
in production ;)

[iannucci, 2015-09-30 02:00:10]

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/d...
File filter/txnBuf/ds_txn.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/d...
filter/txnBuf/ds_txn.go:58: data[i].key = keys[j]
On 2015/09/30 at 01:11:28, Vadim Sh. wrote:
> are you sure it shoud be 'j' here and not 'i'? Or alternatively use getKeys
instead of keys.

er, actually this line needs to be removed.

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/d...
filter/txnBuf/ds_txn.go:102: cb(nil, e)
On 2015/09/30 at 01:11:28, Vadim Sh. wrote:
> what does cb(nil, nil) mean for Put? It's how it's called for "complete" keys
if AllocateIDs failed.

Hm... good question. I should stub in some other errors here so that code
doesn't assume that the put happened. Or, I could make it put the non-error keys
(but that sounds more complex...). Opting for the error.

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/d...
filter/txnBuf/ds_txn.go:138: if len(fq.Project()) == 0 && !fq.KeysOnly() {
On 2015/09/30 at 01:11:28, Vadim Sh. wrote:
> what if fq uses 'limit' or 'skip' or cursor? Worth adding a check.

There is a check for cursors in Run, and runMergedQueries correctly deals with
offset and limit.

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/d...
filter/txnBuf/ds_txn.go:161: needSlimming := len(project) > 0
On 2015/09/30 at 01:11:28, Vadim Sh. wrote:
> nit: just use len(project) > 0 inline. It is using in a single place only.

I guess I was trying to make it more readable... :p

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/q...
File filter/txnBuf/query_merger.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:40: if dedup != nil && dedup(encKey) {
On 2015/09/30 at 01:11:28, Vadim Sh. wrote:
> why have both dedup & filter if they are used identically? Let caller do
"return dedupped() || filtered()" instead.

Hm... ok.

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:62: if itm.err != nil {
On 2015/09/30 at 01:11:28, Vadim Sh. wrote:
> not sure it matters, but iteration function returns (nil, err) only once,
returning (nil, nil) on next invocations. (i.e. error state is not "sticky").

doesn't matter, we bail on the first error.

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/s...
File filter/txnBuf/state.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/s...
filter/txnBuf/state.go:55: if s.keyToSize == nil {
On 2015/09/30 at 01:11:28, Vadim Sh. wrote:
> err... you should probably swap this 'if' with previous line. It will look
less confusing.

done

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/s...
filter/txnBuf/state.go:114: siblingLock sync.Mutex
On 2015/09/30 at 01:11:29, Vadim Sh. wrote:
> hm....

added comment

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/s...
filter/txnBuf/state.go:130: defer parentState.siblingLock.Unlock()
On 2015/09/30 at 01:11:29, Vadim Sh. wrote:
> this is released at the very end of this function, including after cb is
finished.
> 
> Is it what you've intended?

yeah. It's to prevent two transactions on the same level from running at the
same time. So:

RunInTransaction() { // root
  go RunInTransaction()  // A
  go RunInTransaction()  // B
}

A and B would be serialized in some order.

https://chromiumcodereview.appspot.com/1309803004/diff/220001/filter/txnBuf/s...
filter/txnBuf/state.go:258: impossible(err)
On 2015/09/30 at 01:11:29, Vadim Sh. wrote:
> I have a feeling someday we will start seeing this impossible errors showing
up in production ;)

probably! that's why I don't ignore it :) But I don't think that it can actually
happen (as memDS is written today anyhow), so having an if {... panic ...} is
overly verbose. 'impossible' is just a note to say /why/ we'd be panicing here.
Obviously if it happens then we'll have to find out whatever's making it NOT
impossible and squash it.

[Vadim Sh., 2015-09-30 02:09:00]

lgtm, but still waiting for the test with retries

also I didn't really understood all details of query merger. So if you can
harass someone else to review it, it would be nice.

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/ds_txn.go
File filter/txnBuf/ds_txn.go (right):

https://codereview.chromium.org/1309803004/diff/220001/filter/txnBuf/ds_txn.g...
filter/txnBuf/ds_txn.go:161: needSlimming := len(project) > 0
On 2015/09/30 02:00:10, iannucci wrote:
> On 2015/09/30 at 01:11:28, Vadim Sh. wrote:
> > nit: just use len(project) > 0 inline. It is using in a single place only.
> 
> I guess I was trying to make it more readable... :p

Yes. But "slimming" is a peculiar choice of verb for act of "projecting" :) I
understood it only after looking at how it is used => the name didn't really
improve readability (at least for readers like me).

[iannucci, 2015-09-30 03:00:32]

Ok, PTAL. I made all tests retry once except the huge ones. I also split the
tests up to fit in the 20s `-race` deadline, and removed locks during query
execution per our conversation. This also sneaks in a fix to impl/memory that
makes the 'always consistent' mode actually take snapshots when doing a query.

[iannucci, 2015-09-30 03:01:48]

On 2015/09/30 at 03:00:32, iannucci wrote:
> Ok, PTAL. I made all tests retry once except the huge ones. I also split the
tests up to fit in the 20s `-race` deadline, and removed locks during query
execution per our conversation. This also sneaks in a fix to impl/memory that
makes the 'always consistent' mode actually take snapshots when doing a query.

* all Get/Put/Delete tests got a single retry added. I felt that it was enough
to show correctness without slowing things down too much. The tests now take ~1s
on my mac book pro in normal mode, and 16s in -race mode.

[iannucci, 2015-09-30 03:03:31]

https://chromiumcodereview.appspot.com/1309803004/diff/260001/impl/memory/dat...
File impl/memory/datastore_data.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/260001/impl/memory/dat...
impl/memory/datastore_data.go:130: return snap, snap
This was really dumb on my part. `d.head` is the current mutable HEAD state.
getQuerySnaps is supposed to return read-only snapshots.

[iannucci, 2015-09-30 03:27:03]

dnj@chromium.org, estaab@chromium.org, tandrii@chromium.org, anyone want to
additionally review this? :)

[dnj, 2015-09-30 16:35:26]

Sorry for the delay, few comments, mostly nits.

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/c...
File filter/txnBuf/context.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/c...
filter/txnBuf/context.go:23: // transaction to, transitively.
nit: too**

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/d...
File filter/txnBuf/ds_txn.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/d...
filter/txnBuf/ds_txn.go:19: // the transaction to exceed it's alloted number of
entity groups.
nit: allotted**

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/d...
filter/txnBuf/ds_txn.go:130: err := d.state.deleteMulti(keys)
nit: if err := ...; err != nil {...}

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/d...
filter/txnBuf/ds_txn.go:191: newData[p] = data[p]
Is there any check to make sure the user doesn't project on junk fields? If not,
this will populate those fields with nil when real datastore would probably just
not populate them. This can cause "_, ok := ..." checks to respond differently.
Do you care?

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
File filter/txnBuf/query_merger.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:50: for {
No need for "for" loop; you always return.

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:83: if fq.Distinct() {
nit: ditch the "if", just say q = q.Distinct(false).

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:100: // the stack will only include the
information requested by the user; keysonly
nit: keys-only

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:115: return q.Finalize()
All hail editable queries!

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:127: func runMergedQueries(fq *ds.FinalizedQuery,
sizes *sizeTracker, memDS, parentDS ds.RawInterface, cb func(k *ds.Key, data
ds.PropertyMap) bool) error {
nit: This function signature is huge. Can you line-break it somewhere?

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:154: // the original was a normal or keysonly
query, so we need to dedup by keys
nit: keys-only. Also period at the end of comment.

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:207: return err
Should punt toUse before failing from error getting next element.

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
File filter/txnBuf/state.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:172: err = cb(context.WithValue(ctx, dsTxnBufParent,
state))
nit: if err := ...; err != nil { .. }

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:182: // should never be persisted outside of a single
function/query context.
nit: must** never

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:328: proposedState := t.parentState.entState.dup()
Need to lock parentState to dup this?

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:363: wg := sync.WaitGroup{}
WDYT about using parallel.FanOutIn for this?

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:369: if toPut != nil {
nit: len(toPut) > 0

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:383: if toDel != nil {
nit: len(toDel) > 0

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:405: // plus a stringset of all the encoded root keys
that.
nit: root keys that ... what? I must know!

[iannucci, 2015-09-30 17:10:28]

TY, PTAL :)

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/c...
File filter/txnBuf/context.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/c...
filter/txnBuf/context.go:23: // transaction to, transitively.
On 2015/09/30 at 16:35:25, dnj wrote:
> nit: too**

no? I'm `limiting it to` a set, not `limiting it too` a set. :D

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/d...
File filter/txnBuf/ds_txn.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/d...
filter/txnBuf/ds_txn.go:19: // the transaction to exceed it's alloted number of
entity groups.
On 2015/09/30 at 16:35:26, dnj wrote:
> nit: allotted**

done

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/d...
filter/txnBuf/ds_txn.go:130: err := d.state.deleteMulti(keys)
On 2015/09/30 at 16:35:26, dnj wrote:
> nit: if err := ...; err != nil {...}

done

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/d...
filter/txnBuf/ds_txn.go:191: newData[p] = data[p]
On 2015/09/30 at 16:35:26, dnj wrote:
> Is there any check to make sure the user doesn't project on junk fields? If
not, this will populate those fields with nil when real datastore would probably
just not populate them. This can cause "_, ok := ..." checks to respond
differently. Do you care?

projecting on `junk fields` (meaning fields that aren't indexed in any entities)
will cause the query to have an empty result set. 

See:
https://cloud.google.com/appengine/docs/go/datastore/projectionqueries#Go_Lim...
 "Only indexed properties can be projected."

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
File filter/txnBuf/query_merger.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:50: for {
On 2015/09/30 at 16:35:26, dnj wrote:
> No need for "for" loop; you always return.

wat. oh, I think i used to have the filter check in this function. Done.

edit: actually... if I can move the filter check out of this function entirely,
then I can remove the lock, since dedup will only be hit from the main thread...
refactor ENGAGE!

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:83: if fq.Distinct() {
On 2015/09/30 at 16:35:26, dnj wrote:
> nit: ditch the "if", just say q = q.Distinct(false).

done

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:100: // the stack will only include the
information requested by the user; keysonly
On 2015/09/30 at 16:35:26, dnj wrote:
> nit: keys-only

done

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:115: return q.Finalize()
On 2015/09/30 at 16:35:26, dnj wrote:
> All hail editable queries!

indeed :)

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:127: func runMergedQueries(fq *ds.FinalizedQuery,
sizes *sizeTracker, memDS, parentDS ds.RawInterface, cb func(k *ds.Key, data
ds.PropertyMap) bool) error {
On 2015/09/30 at 16:35:26, dnj wrote:
> nit: This function signature is huge. Can you line-break it somewhere?

done

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:154: // the original was a normal or keysonly
query, so we need to dedup by keys
On 2015/09/30 at 16:35:26, dnj wrote:
> nit: keys-only. Also period at the end of comment.

done

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/q...
filter/txnBuf/query_merger.go:207: return err
On 2015/09/30 at 16:35:26, dnj wrote:
> Should punt toUse before failing from error getting next element.

ah, yeah, ok.

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
File filter/txnBuf/state.go (right):

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:172: err = cb(context.WithValue(ctx, dsTxnBufParent,
state))
On 2015/09/30 at 16:35:26, dnj wrote:
> nit: if err := ...; err != nil { .. }

done

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:182: // should never be persisted outside of a single
function/query context.
On 2015/09/30 at 16:35:26, dnj wrote:
> nit: must** never

done

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:328: proposedState := t.parentState.entState.dup()
On 2015/09/30 at 16:35:26, dnj wrote:
> Need to lock parentState to dup this?

yep. done

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:363: wg := sync.WaitGroup{}
On 2015/09/30 at 16:35:26, dnj wrote:
> WDYT about using parallel.FanOutIn for this?

wow much better. y I do it rong?

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:369: if toPut != nil {
On 2015/09/30 at 16:35:26, dnj wrote:
> nit: len(toPut) > 0

ya

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:376: i++
hey look! a boog! i is incremented before assigned to the multierror!

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:393: dErr = mErr.Get()
hey look! a boog! dErr is blindly overwritten!

https://chromiumcodereview.appspot.com/1309803004/diff/300001/filter/txnBuf/s...
filter/txnBuf/state.go:405: // plus a stringset of all the encoded root keys
that.
On 2015/09/30 at 16:35:26, dnj wrote:
> nit: root keys that ... what? I must know!

doone

[iannucci, 2015-09-30 17:10:41]

Oh, and I got rid of another mutex! woo!

[dnj, 2015-09-30 17:27:30]

Nice, clean up some of the crazy parts. lgtm

[iannucci, 2015-09-30 17:39:18]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-09-30 17:39:19]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1309803004/#ps300002 (title: "move
errcheck code")

[commit-bot: I haz the power, 2015-09-30 17:39:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309803004/300002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309803004/300002

[commit-bot: I haz the power, 2015-09-30 17:44:04]

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

[commit-bot: I haz the power, 2015-09-30 17:44:05]

Try jobs failed on following builders:
  Luci-GAE Presubmit on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Luci-GAE%20Presubmit/bu...)

[iannucci, 2015-09-30 21:09:07]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-09-30 21:09:07]

The patchset sent to the CQ was uploaded after l-g-t-m from dnj@chromium.org,
vadimsh@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1309803004/#ps350001 (title: "change pcg
timeout and codereview server to crcr.as.com")

[commit-bot: I haz the power, 2015-09-30 21:09:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309803004/350001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309803004/350001

[commit-bot: I haz the power, 2015-09-30 21:11:15]

Committed patchset #19 (id:350001) as
https://github.com/luci/gae/commit/ce4a45029c9c317224c7eb2182b5eddd91f1d5dc