Fix a bug where deletions weren't updating the raw Kind index.

Fix a bug where deletions weren't updating the raw Kind index.

This manifested as "limit" and "offset" fields being seemingly ignored.
The real reason for this is because the query iterator was (correctly)
skipping index entries that weren't present in head, but counting them
against the limit/offset.

BUG=None
TEST=None

Committed: https://github.com/luci/gae/commit/04506f4321aedd2dd1e8d08b68e1ff27a8a82894

[dnj, 2016-11-11 08:33:17]

Patchset #1 (id:1) has been deleted

[dnj, 2016-11-11 08:37:52]

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

[dnj, 2016-11-11 08:37:53]

PTAL. I think this is the right way to do things. LMK if I'm barking up the
wrong tree.

https://codereview.chromium.org/2498463003/diff/20001/impl/memory/context.go
File impl/memory/context.go (left):

https://codereview.chromium.org/2498463003/diff/20001/impl/memory/context.go#...
impl/memory/context.go:150: memContextKey memContextKeyType
golint error. This was actually a bug - glad nobody else used the number 1 :x

I took the opportunity to make this a string pointer.

https://codereview.chromium.org/2498463003/diff/20001/impl/memory/datastore_q...
File impl/memory/datastore_query_execution.go (left):

https://codereview.chromium.org/2498463003/diff/20001/impl/memory/datastore_q...
impl/memory/datastore_query_execution.go:169: if len(fq.Project()) == 0 &&
!fq.KeysOnly() {
So maybe I'm reading things wrong, but I think Count should behave the same as
Get. Making it KeysOnly here allows it to return keys from stale indexes that
wouldn't be returned by Get, making it incorrect.

https://codereview.chromium.org/2498463003/diff/20001/impl/memory/datastore_q...
File impl/memory/datastore_query_execution.go (right):

https://codereview.chromium.org/2498463003/diff/20001/impl/memory/datastore_q...
impl/memory/datastore_query_execution.go:128: return errQueryEntrySkipped
This is the source of the problem. If the callback is skipped b/c entity doesn't
exist at HEAD, "limit" and "offset" are still advanced, counting against it.

https://codereview.chromium.org/2498463003/diff/20001/impl/memory/datastore_q...
File impl/memory/datastore_query_execution_test.go (left):

https://codereview.chromium.org/2498463003/diff/20001/impl/memory/datastore_q...
impl/memory/datastore_query_execution_test.go:255: // count counts from the
index with KeysOnly and so counts the deleted
(This is no longer true, since we don't auto-KeysOnly queries for Count).

https://codereview.chromium.org/2498463003/diff/20001/impl/memory/datastore_q...
File impl/memory/datastore_query_execution_test.go (right):

https://codereview.chromium.org/2498463003/diff/20001/impl/memory/datastore_q...
impl/memory/datastore_query_execution_test.go:28: func enable() bool {
This is something useful for debugging specific test cases. Basically add this
to your query and the code will *only* execute that query. Much easier to
isolate than massive comment blocks.

[dnj, 2016-11-11 19:29:08]

Description was changed from

==========
impl/memory: query limits/offsets count deleted

Fix a bug in impl/memory's query execution where entities that are
delete in HEAD, but not deleted the index under query, are silently
dropped but are still counted against that query's offset/limit.

Removed "Count" optimization to use a keys-only query when not
projecting, since this should behave like a normal Get and operate
against HEAD instead of a potentially stale index. This was replaced
with another optimization to avoid wasting deserialization time for
keys-only entities.

Added some tests for the regression case and updated some othe tests in
light of the Count optimization removal.

Also fix a golint error about Context key type.

BUG=None
TEST=unit
==========

to

==========
impl/memory: query limits/offsets count deleted

Fix a bug in impl/memory's query execution where entities that are
delete in HEAD, but not deleted the index under query, are silently
dropped but are still counted against that query's offset/limit.

Removed "Count" optimization to use a keys-only query when not
projecting, since this should behave like a normal Query and operate
against HEAD instead of a potentially stale index. This was replaced
with another optimization to avoid wasting deserialization time for
keys-only entities.

Added some tests for the regression case and updated some othe tests in
light of the Count optimization removal.

Also fix a golint error about Context key type.

BUG=None
TEST=unit
==========

[dnj, 2016-11-11 23:59:09]

Description was changed from

==========
impl/memory: query limits/offsets count deleted

Fix a bug in impl/memory's query execution where entities that are
delete in HEAD, but not deleted the index under query, are silently
dropped but are still counted against that query's offset/limit.

Removed "Count" optimization to use a keys-only query when not
projecting, since this should behave like a normal Query and operate
against HEAD instead of a potentially stale index. This was replaced
with another optimization to avoid wasting deserialization time for
keys-only entities.

Added some tests for the regression case and updated some othe tests in
light of the Count optimization removal.

Also fix a golint error about Context key type.

BUG=None
TEST=unit
==========

to

==========
Fix 'impl/memory' context key.

Fix a bug where deletions weren't updating the raw Kind index.

This manifested as "limit" and "offset" fields being seemingly ignored.
The real reason for this is because the query iterator was (correctly)
skipping index entries that weren't present in head, but counting them
against the limit/offset.

BUG=None
TEST=None
==========

[iannucci, 2016-11-12 00:05:47]

good find :) lgtm

https://codereview.chromium.org/2498463003/diff/40001/impl/memory/datastore_i...
File impl/memory/datastore_index.go (right):

https://codereview.chromium.org/2498463003/diff/40001/impl/memory/datastore_i...
impl/memory/datastore_index.go:262: func updateIndexes(store memStore, key
*ds.Key, oldEnt, newEnt ds.PropertyMap) {
add comment mentioning that nil has special meaning vs just an empty property
map.

https://codereview.chromium.org/2498463003/diff/40001/impl/memory/datastore_q...
File impl/memory/datastore_query_execution_test.go (right):

https://codereview.chromium.org/2498463003/diff/40001/impl/memory/datastore_q...
impl/memory/datastore_query_execution_test.go:553: }
derp

https://codereview.chromium.org/2498463003/diff/40001/impl/memory/gkvlite_uti...
File impl/memory/gkvlite_utils.go (right):

https://codereview.chromium.org/2498463003/diff/40001/impl/memory/gkvlite_uti...
impl/memory/gkvlite_utils.go:36: wg := &sync.WaitGroup{}
del wg too?

[iannucci, 2016-11-12 00:19:12]

lgtm

https://codereview.chromium.org/2498463003/diff/90001/impl/memory/datastore_i...
File impl/memory/datastore_index.go (right):

https://codereview.chromium.org/2498463003/diff/90001/impl/memory/datastore_i...
impl/memory/datastore_index.go:51: // specifically deletion.
typo?

[dnj, 2016-11-12 00:20:53]

The CQ bit was checked by dnj@chromium.org

[dnj, 2016-11-12 00:20:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2498463003/#ps110001
(title: "Comments, review.")

[commit-bot: I haz the power, 2016-11-12 00:20:58]

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

[dnj, 2016-11-12 00:22:47]

The CQ bit was unchecked by dnj@chromium.org

[dnj, 2016-11-12 00:22:58]

Description was changed from

==========
Fix 'impl/memory' context key.

Fix a bug where deletions weren't updating the raw Kind index.

This manifested as "limit" and "offset" fields being seemingly ignored.
The real reason for this is because the query iterator was (correctly)
skipping index entries that weren't present in head, but counting them
against the limit/offset.

BUG=None
TEST=None
==========

to

==========
Fix a bug where deletions weren't updating the raw Kind index.

This manifested as "limit" and "offset" fields being seemingly ignored.
The real reason for this is because the query iterator was (correctly)
skipping index entries that weren't present in head, but counting them
against the limit/offset.

BUG=None
TEST=None
==========

[dnj, 2016-11-12 00:23:01]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-11-12 00:23:09]

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-12 00:27:15]

Description was changed from

==========
Fix a bug where deletions weren't updating the raw Kind index.

This manifested as "limit" and "offset" fields being seemingly ignored.
The real reason for this is because the query iterator was (correctly)
skipping index entries that weren't present in head, but counting them
against the limit/offset.

BUG=None
TEST=None
==========

to

==========
Fix a bug where deletions weren't updating the raw Kind index.

This manifested as "limit" and "offset" fields being seemingly ignored.
The real reason for this is because the query iterator was (correctly)
skipping index entries that weren't present in head, but counting them
against the limit/offset.

BUG=None
TEST=None

Committed:
https://github.com/luci/gae/commit/04506f4321aedd2dd1e8d08b68e1ff27a8a82894
==========

[commit-bot: I haz the power, 2016-11-12 00:27:16]

Committed patchset #6 (id:110001) as
https://github.com/luci/gae/commit/04506f4321aedd2dd1e8d08b68e1ff27a8a82894