Add more safety checks.

Add more safety checks.

A rootless index has no consistency guarantee. So ensure that the returned value
is good. There has been at least one case on production where the index returned
an object matching properties_hash != None but had failed.

R=vadimsh@chromium.org
BUG=chromium:642511
Committed: https://github.com/luci/luci-py/commit/62dac3d6729d3389437fb2710e3191eb8834f3db

[M-A Ruel, 2016-08-31 01:49:16]

That should help.

[M-A Ruel, 2016-08-31 16:30:33]

Description was changed from

==========
Add more safety checks.

R=vadimsh@chromium.org
BUG=chromium:642511
==========

to

==========
Add more safety checks.

A rootless index has no consistency guarantee. So ensure that the returned value
is good. There has been at least one case on production where the index returned
an object matching properties_hash != None but had failed.

R=vadimsh@chromium.org
BUG=chromium:642511
==========

[Vadim Sh., 2016-08-31 19:46:06]

https://codereview.chromium.org/2298053002/diff/20001/appengine/swarming/serv...
File appengine/swarming/server/task_scheduler.py (right):

https://codereview.chromium.org/2298053002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:478: for dupe_summary in
cls.query(cls.properties_hash==h):
nit:

for i, dupe_summary in iterate(....):
  ...

(instead of 'count')

https://codereview.chromium.org/2298053002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:478: for dupe_summary in
cls.query(cls.properties_hash==h):
no order by key anymore?

https://codereview.chromium.org/2298053002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:483: if count == 3:
Then also use smaller batch_size (it is 20 by default, so cls.query will fetch
20 results no matter what).

https://codereview.chromium.org/2298053002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:496: # Setting task.queue_number to
None removes it from the scheduling.
nit: break out of the loop here and do the rest of the work outside of 'for'
loop

[M-A Ruel, 2016-08-31 20:26:30]

https://codereview.chromium.org/2298053002/diff/20001/appengine/swarming/serv...
File appengine/swarming/server/task_scheduler.py (right):

https://codereview.chromium.org/2298053002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:478: for dupe_summary in
cls.query(cls.properties_hash==h):
On 2016/08/31 19:46:06, Vadim Sh. wrote:
> no order by key anymore?

Humm not sure why I had removed it, added back.

https://codereview.chromium.org/2298053002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:478: for dupe_summary in
cls.query(cls.properties_hash==h):
On 2016/08/31 19:46:06, Vadim Sh. wrote:
> nit:
> 
> for i, dupe_summary in iterate(....):
>   ...
> 
> (instead of 'count')

Done.

https://codereview.chromium.org/2298053002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:483: if count == 3:
On 2016/08/31 19:46:06, Vadim Sh. wrote:
> Then also use smaller batch_size (it is 20 by default, so cls.query will fetch
> 20 results no matter what).

Done.

https://codereview.chromium.org/2298053002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:496: # Setting task.queue_number to
None removes it from the scheduling.
On 2016/08/31 19:46:06, Vadim Sh. wrote:
> nit: break out of the loop here and do the rest of the work outside of 'for'
> loop

Refactored searching code into a function.

https://codereview.chromium.org/2298053002/diff/40001/appengine/swarming/serv...
File appengine/swarming/server/task_scheduler.py (right):

https://codereview.chromium.org/2298053002/diff/40001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:420: for i, dupe_summary in
enumerate(q.iter(batch_size=1)):
Using batch_size=1 since the general case is to only read exactly one item. The
loop is only when the index becomes really badly inconsistent.

[Vadim Sh., 2016-08-31 21:43:43]

lgtm

[M-A Ruel, 2016-08-31 21:53:15]

The CQ bit was checked by maruel@chromium.org

[commit-bot: I haz the power, 2016-08-31 21:53:24]

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-08-31 21:57:23]

Description was changed from

==========
Add more safety checks.

A rootless index has no consistency guarantee. So ensure that the returned value
is good. There has been at least one case on production where the index returned
an object matching properties_hash != None but had failed.

R=vadimsh@chromium.org
BUG=chromium:642511
==========

to

==========
Add more safety checks.

A rootless index has no consistency guarantee. So ensure that the returned value
is good. There has been at least one case on production where the index returned
an object matching properties_hash != None but had failed.

R=vadimsh@chromium.org
BUG=chromium:642511

Committed:
https://github.com/luci/luci-py/commit/62dac3d6729d3389437fb2710e3191eb8834f3db
==========

[commit-bot: I haz the power, 2016-08-31 21:57:24]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-py/commit/62dac3d6729d3389437fb2710e3191eb8834f3db