Use a datastore batcher for build queries.

Use a datastore batcher for build queries.

Queries are timing out. This is because processing elements is
CPU-intensive, and datastore queries have a maximum lifetime of 30
seconds. Use batching to break the single query/deserialize process into
a series of consecutive queries so any individual query doesn't run into
the timeout limit.

Note that really high limits will still bump into the actual AppEngine
request timeout.

BUG=chromium:687236
TEST=None
R=estaab@chromium.org, hinoka@chromium.org

Review-Url: https://codereview.chromium.org/2668763002
Committed: https://github.com/luci/luci-go/commit/b7c33500af911388323e6e612e933ea0f7f57888

[hinoka, 2017-01-31 19:05:32]

ah for some reason i thought datastore batched automatically like ndb did.

this lgtm but just one comment/suggestion.

https://codereview.chromium.org/2668763002/diff/1/milo/appengine/buildbot/dat...
File milo/appengine/buildbot/datastore.go (right):

https://codereview.chromium.org/2668763002/diff/1/milo/appengine/buildbot/dat...
milo/appengine/buildbot/datastore.go:23: func getBuildQueryBatcher(c
context.Context) *ds.Batcher {
Would it make more sense to put this in the ds datastore module?

[estaab, 2017-01-31 19:16:53]

Awesome! Seems like a straightforward improvement.

[estaab, 2017-01-31 19:16:56]

lgtm

[dnj, 2017-01-31 19:19:39]

https://codereview.chromium.org/2668763002/diff/1/milo/appengine/buildbot/dat...
File milo/appengine/buildbot/datastore.go (right):

https://codereview.chromium.org/2668763002/diff/1/milo/appengine/buildbot/dat...
milo/appengine/buildbot/datastore.go:23: func getBuildQueryBatcher(c
context.Context) *ds.Batcher {
On 2017/01/31 19:05:32, hinoka wrote:
> Would it make more sense to put this in the ds datastore module?

In the "ds" module, as in "luci/gae"? Batcher is a construct in luci/gae
already.

It's not the default behavior because it's potentially imperfect. It uses
cursors behind the scenes to resume datastore queries, so in some circumstances
the query result may not match the result of a full GetAll. It's not built-in
because we want the user to elect to have this imperfection, but it should be
fine for Milo's purposes, though. 

Also, the batch size is difficult to estimate. Here, it's MaxQueryDuration /
((QueryRPCOverhead * NumRPC) + (DeserializeOverhead * NumEntities)). Lots of
these are unknowns, and building a system to automatically adapt is non-trivial
and also doesn't exist ATM.

[dnj, 2017-01-31 19:20:40]

On 2017/01/31 19:19:39, dnj wrote:
>
https://codereview.chromium.org/2668763002/diff/1/milo/appengine/buildbot/dat...
> File milo/appengine/buildbot/datastore.go (right):
> 
>
https://codereview.chromium.org/2668763002/diff/1/milo/appengine/buildbot/dat...
> milo/appengine/buildbot/datastore.go:23: func getBuildQueryBatcher(c
> context.Context) *ds.Batcher {
> On 2017/01/31 19:05:32, hinoka wrote:
> > Would it make more sense to put this in the ds datastore module?
> 
> In the "ds" module, as in "luci/gae"? Batcher is a construct in luci/gae
> already.
> 
> It's not the default behavior because it's potentially imperfect. It uses
> cursors behind the scenes to resume datastore queries, so in some
circumstances
> the query result may not match the result of a full GetAll. It's not built-in
> because we want the user to elect to have this imperfection, but it should be
> fine for Milo's purposes, though. 
> 
> Also, the batch size is difficult to estimate. Here, it's MaxQueryDuration /
> ((QueryRPCOverhead * NumRPC) + (DeserializeOverhead * NumEntities)). Lots of
> these are unknowns, and building a system to automatically adapt is
non-trivial
> and also doesn't exist ATM.

Actually, I think that should say:
(MaxQueryDuration - ((QueryRPCOverhead * NumRPC)) / (DeserializeOverhead *
NumEntities)

[dnj, 2017-01-31 19:20:48]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2017-01-31 19:20:56]

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

[hinoka, 2017-01-31 19:27:35]

Makes sense, thanks for the explanation.

[commit-bot: I haz the power, 2017-01-31 19:27:53]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485890448371460, "parent_rev":
"9f7d2643fb13fae058d3dad28de04c1ccac02391", "commit_rev":
"b7c33500af911388323e6e612e933ea0f7f57888"}

[commit-bot: I haz the power, 2017-01-31 19:27:57]

Description was changed from

==========
Use a datastore batcher for build queries.

Queries are timing out. This is because processing elements is
CPU-intensive, and datastore queries have a maximum lifetime of 30
seconds. Use batching to break the single query/deserialize process into
a series of consecutive queries so any individual query doesn't run into
the timeout limit.

Note that really high limits will still bump into the actual AppEngine
request timeout.

BUG=chromium:687236
TEST=None
R=estaab@chromium.org, hinoka@chromium.org
==========

to

==========
Use a datastore batcher for build queries.

Queries are timing out. This is because processing elements is
CPU-intensive, and datastore queries have a maximum lifetime of 30
seconds. Use batching to break the single query/deserialize process into
a series of consecutive queries so any individual query doesn't run into
the timeout limit.

Note that really high limits will still bump into the actual AppEngine
request timeout.

BUG=chromium:687236
TEST=None
R=estaab@chromium.org, hinoka@chromium.org

Review-Url: https://codereview.chromium.org/2668763002
Committed:
https://github.com/luci/luci-go/commit/b7c33500af911388323e6e612e933ea0f7f57888
==========

[commit-bot: I haz the power, 2017-01-31 19:27:58]

Committed patchset #1 (id:1) as
https://github.com/luci/luci-go/commit/b7c33500af911388323e6e612e933ea0f7f57888