task_queues: Add more scaffolding

task_queues: Add more scaffolding

Do not add the logic yet but add the cron job and task queues, to make follow up
CL more readable. This code adds all the needed models, public APIs and
docstrings.

No real functionality change except new cron job and task queue that essentially
do nothing.

Partially update task_to_run_test.py in preparation to running task queues; more
modifications are needed but the current one have no logic effect yet.

R=vadimsh@chromium.org
BUG=704236
Review-Url: https://codereview.chromium.org/2832203002
Committed: https://github.com/luci/luci-py/commit/f7b1beccb66ef34fc7b183e4e6e1287ec0e72658

[M-A Ruel, 2017-04-21 14:02:35]

Description was changed from

==========
task_queues: Add more scaffolding

Do not add the logic yet but add the cron job and task queues, to make follow up
CL more readable. This code adds all the needed models, public APIs and
docstrings.

Partially update task_to_run_test.py in preparation to running task queues; more
modifications are needed but the current one have no logic effect yet.

R=vadimsh@chromium.org
BUG=704236
==========

to

==========
task_queues: Add more scaffolding

Do not add the logic yet but add the cron job and task queues, to make follow up
CL more readable. This code adds all the needed models, public APIs and
docstrings.

No real functionality change except new cron job and task queue that essentially
do nothing.

Partially update task_to_run_test.py in preparation to running task queues; more
modifications are needed but the current one have no logic effect yet.

R=vadimsh@chromium.org
BUG=704236
==========

[M-A Ruel, 2017-04-21 14:02:43]

Patchset #1 (id:1) has been deleted

[M-A Ruel, 2017-04-21 14:07:45]

Patchset #1 (id:20001) has been deleted

[M-A Ruel, 2017-04-21 14:16:46]

Patchset #1 (id:40001) has been deleted

[M-A Ruel, 2017-04-21 14:16:53]

The important part of this CL is really the docstrings, feel free to comment if
anything is nebulous. Obviously it'll be clearer with code but since this is a
very sensible part of the code I want the comments to be enlightening

[Vadim Sh., 2017-04-26 18:26:25]

lgtm with questions

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
File appengine/swarming/server/bot_management.py (right):

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/bot_management.py:24: |BotTaskDimensions  | ...
|BotTaskDimensions  |
do this entities created/deleted separately or always as a whole batch? Maybe
instead of keeping N entities and using ancestor keys only queries (that are NOT
cached), it will be possible to keep all dimension hashes in a single entity (as
a non-indexed repeated field) and fetch it at once (relying on ndb cache).

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
File appengine/swarming/server/task_queues.py (right):

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:106: Parent is TaskDimensionsRoot
Any reason we use parents instead of storing each entity separately? I can
imagine situations where 1 QPS here may be limiting.

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:136: frozenset('%s:%s' % (k, v) for k,
v in dimensions.iteritems()))
remove frozenset, since confirm_flat already converts iterable to a set

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:140: x = frozenset(dimensions_flat)
'fronzen' here is not needed, 'x' is never returned or stored

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:147: return any(s.match(bot_dimensions)
for s in self.sets)
strictly speaking, this can be optimized by using some tree-like structure, but
it's probably not worth it

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:159: dimensions_json must already be
encoded as json.
I hope you changed it to accept dimensions as is and serialize it here instead.

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:192: assert isinstance(bot_id,
unicode), repr(bot_id)
unicode, really? Do we have bots with non-ascii names? It's not possible.

[M-A Ruel, 2017-04-26 21:07:07]

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
File appengine/swarming/server/bot_management.py (right):

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/bot_management.py:24: |BotTaskDimensions  | ...
|BotTaskDimensions  |
On 2017/04/26 18:26:24, Vadim Sh. wrote:
> do this entities created/deleted separately or always as a whole batch? Maybe
> instead of keeping N entities and using ancestor keys only queries (that are
NOT
> cached), it will be possible to keep all dimension hashes in a single entity
(as
> a non-indexed repeated field) and fetch it at once (relying on ndb cache).

That's an excellent question and Michael had raised this too.

In practice I think it doesn't matter.

If I were to do one entity, this would require a transaction. The current design
doesn't require one. So this is actually faster for writes.

For reads, it keeps a cache in memcache that is coherent, so it's not a perf
problem in practice.

If it ever becomes a problem, I could switch over, this would be slightly
breaking change but not too breaking. I prefer to try to not require transaction
initially.

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
File appengine/swarming/server/task_queues.py (right):

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:106: Parent is TaskDimensionsRoot
On 2017/04/26 18:26:24, Vadim Sh. wrote:
> Any reason we use parents instead of storing each entity separately? I can
> imagine situations where 1 QPS here may be limiting.

It's for coherency. There's a risk if inside a pool there's a *lot* of
variations (thousands) but the key point is to have no transaction.

The only transaction is in the single threaded cron job to remove stale
entities.

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:136: frozenset('%s:%s' % (k, v) for k,
v in dimensions.iteritems()))
On 2017/04/26 18:26:24, Vadim Sh. wrote:
> remove frozenset, since confirm_flat already converts iterable to a set

Done.

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:140: x = frozenset(dimensions_flat)
On 2017/04/26 18:26:24, Vadim Sh. wrote:
> 'fronzen' here is not needed, 'x' is never returned or stored

It's meant as a "reader expectation" signal. I think there is perf implication
but I'm not sure.

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:147: return any(s.match(bot_dimensions)
for s in self.sets)
On 2017/04/26 18:26:24, Vadim Sh. wrote:
> strictly speaking, this can be optimized by using some tree-like structure,
but
> it's probably not worth it

Acknowledged.

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:159: dimensions_json must already be
encoded as json.
On 2017/04/26 18:26:24, Vadim Sh. wrote:
> I hope you changed it to accept dimensions as is and serialize it here
instead.


Oh yes, I had forgot to backport to this CL. Done for completeness.

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:192: assert isinstance(bot_id,
unicode), repr(bot_id)
On 2017/04/26 18:26:24, Vadim Sh. wrote:
> unicode, really? Do we have bots with non-ascii names? It's not possible.

everything is unicode, I just wanted to make sure I didn't mess the type.

[Vadim Sh., 2017-04-26 21:24:56]

still lgmt, it's all not essential now

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
File appengine/swarming/server/bot_management.py (right):

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/bot_management.py:24: |BotTaskDimensions  | ...
|BotTaskDimensions  |
On 2017/04/26 21:07:07, M-A Ruel wrote:
> On 2017/04/26 18:26:24, Vadim Sh. wrote:
> > do this entities created/deleted separately or always as a whole batch?
Maybe
> > instead of keeping N entities and using ancestor keys only queries (that are
> NOT
> > cached), it will be possible to keep all dimension hashes in a single entity
> (as
> > a non-indexed repeated field) and fetch it at once (relying on ndb cache).
> 
> That's an excellent question and Michael had raised this too.
> 
> In practice I think it doesn't matter.
> 
> If I were to do one entity, this would require a transaction. The current
design
> doesn't require one. So this is actually faster for writes.
> 
It will require a transaction only if we add dimension_hashes one by one (and
not, for example, rebuild them all at once from scratch). I haven't seen the
code yet.

> For reads, it keeps a cache in memcache that is coherent, so it's not a perf
> problem in practice.
ndb.Qurery() results are not memcached. Fetching all BotTaskDimensions entities
will always hit datastore.

> 
> If it ever becomes a problem, I could switch over, this would be slightly
> breaking change but not too breaking. I prefer to try to not require
transaction
> initially.

ok

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
File appengine/swarming/server/task_queues.py (right):

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/task_queues.py:140: x = frozenset(dimensions_flat)
On 2017/04/26 21:07:07, M-A Ruel wrote:
> On 2017/04/26 18:26:24, Vadim Sh. wrote:
> > 'fronzen' here is not needed, 'x' is never returned or stored
> 
> It's meant as a "reader expectation" signal. 
This is confusing reader expectation signal :-/ fronzenset is used when
returning someing (or storing something) to indicate it is constant. Here it's
neither returned, nor stored.

[M-A Ruel, 2017-04-26 21:30:31]

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
File appengine/swarming/server/bot_management.py (right):

https://codereview.chromium.org/2832203002/diff/60001/appengine/swarming/serv...
appengine/swarming/server/bot_management.py:24: |BotTaskDimensions  | ...
|BotTaskDimensions  |
On 2017/04/26 21:24:55, Vadim Sh. wrote:
> On 2017/04/26 21:07:07, M-A Ruel wrote:
> > On 2017/04/26 18:26:24, Vadim Sh. wrote:
> > > do this entities created/deleted separately or always as a whole batch?
> Maybe
> > > instead of keeping N entities and using ancestor keys only queries (that
are
> > NOT
> > > cached), it will be possible to keep all dimension hashes in a single
entity
> > (as
> > > a non-indexed repeated field) and fetch it at once (relying on ndb cache).
> > 
> > That's an excellent question and Michael had raised this too.
> > 
> > In practice I think it doesn't matter.
> > 
> > If I were to do one entity, this would require a transaction. The current
> design
> > doesn't require one. So this is actually faster for writes.
> > 
> It will require a transaction only if we add dimension_hashes one by one (and
> not, for example, rebuild them all at once from scratch). I haven't seen the
> code yet.

Actually each dimensions_hash has its own expiration time so it needs to be
updated more frequently. Anyhow, I'm keeping this in mind.


> > For reads, it keeps a cache in memcache that is coherent, so it's not a perf
> > problem in practice.
> ndb.Qurery() results are not memcached. Fetching all BotTaskDimensions
entities
> will always hit datastore.

I meant I'm caching the ndb.Query result manually. It's like 5 lines.

[M-A Ruel, 2017-04-26 21:30:42]

The CQ bit was checked by maruel@chromium.org

[M-A Ruel, 2017-04-26 21:30:43]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/2832203002/#ps80001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-04-26 21:30:54]

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, 2017-04-26 21:34:26]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493242242999680,
"parent_rev": "dc8358f0fe0ef542fb3a5334e74cadecca52963d", "commit_rev":
"f7b1beccb66ef34fc7b183e4e6e1287ec0e72658"}

[commit-bot: I haz the power, 2017-04-26 21:34:28]

Description was changed from

==========
task_queues: Add more scaffolding

Do not add the logic yet but add the cron job and task queues, to make follow up
CL more readable. This code adds all the needed models, public APIs and
docstrings.

No real functionality change except new cron job and task queue that essentially
do nothing.

Partially update task_to_run_test.py in preparation to running task queues; more
modifications are needed but the current one have no logic effect yet.

R=vadimsh@chromium.org
BUG=704236
==========

to

==========
task_queues: Add more scaffolding

Do not add the logic yet but add the cron job and task queues, to make follow up
CL more readable. This code adds all the needed models, public APIs and
docstrings.

No real functionality change except new cron job and task queue that essentially
do nothing.

Partially update task_to_run_test.py in preparation to running task queues; more
modifications are needed but the current one have no logic effect yet.

R=vadimsh@chromium.org
BUG=704236

Review-Url: https://codereview.chromium.org/2832203002
Committed:
https://github.com/luci/luci-py/commit/f7b1beccb66ef34fc7b183e4e6e1287ec0e72658
==========

[commit-bot: I haz the power, 2017-04-26 21:34:29]

Committed patchset #2 (id:80001) as
https://github.com/luci/luci-py/commit/f7b1beccb66ef34fc7b183e4e6e1287ec0e72658