[Findit] Reduce redundant ndb reads by querying necessary entities ahead of time and share them amo…

[Findit] Reduce redundant ndb reads by querying necessary entities ahead of time and share them among failures.

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/0d99c79a624c624b5bd42dfb0a6bc08838907b32

[chanli, 2016-10-19 18:20:38]

chanli@chromium.org changed reviewers:
+ lijeffrey@chromium.org, stgao@chromium.org

[chanli, 2016-10-19 18:20:39]

ptal

[lijeffrey, 2016-10-19 20:35:25]

https://chromiumcodereview.appspot.com/2439553002/diff/20001/appengine/findit...
File appengine/findit/findit_api.py (right):

https://chromiumcodereview.appspot.com/2439553002/diff/20001/appengine/findit...
appengine/findit/findit_api.py:301: else:
nit: add a comment here for the type of structure of step_map

https://chromiumcodereview.appspot.com/2439553002/diff/20001/appengine/findit...
appengine/findit/findit_api.py:327: else:
nit: same here

https://chromiumcodereview.appspot.com/2439553002/diff/20001/appengine/findit...
appengine/findit/findit_api.py:335: self, step_name, test_name,
failure_result_map, swarming_tasks, try_jobs):
nit: please add a docstring for this function

https://chromiumcodereview.appspot.com/2439553002/diff/20001/appengine/findit...
appengine/findit/findit_api.py:346: # Get the try job for the step/ test.
nit: remove extra space before 'test' and add 1 empty line before this comment

[stgao, 2016-10-22 00:29:03]

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
File appengine/findit/findit_api.py (right):

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:242: return False
So for tests, there are three status:
1. Reliable
2. Flaky
3. Unclassified

It seems Reliable + Unclassified are treated as Reliable here.

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:300: WfSwarmingTask.Get(*step_map.split('/'),
step_name=step_name))
again: encode & decode should have a function.

[chanli, 2016-10-24 18:42:34]

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
File appengine/findit/findit_api.py (right):

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:242: return False
On 2016/10/22 00:29:03, stgao wrote:
> So for tests, there are three status:
> 1. Reliable
> 2. Flaky
> 3. Unclassified
> 
> It seems Reliable + Unclassified are treated as Reliable here.

We don't have swarming tasks and try jobs for unclassified tests, so we cannot
say they are flaky; and if we have heuristic results for them, we have to treat
them as reliable tests and display what we found on SoM. What makes it better is
that we now have confidence score, so it should provide some level of guidance
to sheriffs about how trustworthy the result is.

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:300: WfSwarmingTask.Get(*step_map.split('/'),
step_name=step_name))
On 2016/10/22 00:29:03, stgao wrote:
> again: encode & decode should have a function.

Done.

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:301: else:
On 2016/10/19 20:35:25, lijeffrey wrote:
> nit: add a comment here for the type of structure of step_map

Added Args in docstring.

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:327: else:
On 2016/10/19 20:35:24, lijeffrey wrote:
> nit: same here

Done.

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:335: self, step_name, test_name,
failure_result_map, swarming_tasks, try_jobs):
On 2016/10/19 20:35:25, lijeffrey wrote:
> nit: please add a docstring for this function

Done.

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:346: # Get the try job for the step/ test.
On 2016/10/19 20:35:24, lijeffrey wrote:
> nit: remove extra space before 'test' and add 1 empty line before this comment

Done.

[stgao, 2016-10-25 17:24:04]

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
File appengine/findit/findit_api.py (right):

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:242: return False
On 2016/10/24 18:42:34, chanli wrote:
> On 2016/10/22 00:29:03, stgao wrote:
> > So for tests, there are three status:
> > 1. Reliable
> > 2. Flaky
> > 3. Unclassified
> > 
> > It seems Reliable + Unclassified are treated as Reliable here.
> 
> We don't have swarming tasks and try jobs for unclassified tests, so we cannot
> say they are flaky; 

Sorry for the confusion. My question here is: what "False" mean here? If it
means reliable, why Unclassified ones are treated as reliable even we don't know
for sure?

> and if we have heuristic results for them, we have to treat
> them as reliable tests 

As it is determined to be flaky by Swarm rerun, what's the consideration for a
flaky test to be treated as reliable one here?

> and display what we found on SoM. What makes it better is
> that we now have confidence score, so it should provide some level of guidance
> to sheriffs about how trustworthy the result is.

[chanli, 2016-10-26 03:37:00]

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
File appengine/findit/findit_api.py (right):

https://codereview.chromium.org/2439553002/diff/20001/appengine/findit/findit...
appengine/findit/findit_api.py:242: return False
On 2016/10/25 17:24:04, stgao wrote:
> On 2016/10/24 18:42:34, chanli wrote:
> > On 2016/10/22 00:29:03, stgao wrote:
> > > So for tests, there are three status:
> > > 1. Reliable
> > > 2. Flaky
> > > 3. Unclassified
> > > 
> > > It seems Reliable + Unclassified are treated as Reliable here.
> > 
> > We don't have swarming tasks and try jobs for unclassified tests, so we
cannot
> > say they are flaky; 
> 
> Sorry for the confusion. My question here is: what "False" mean here? If it
> means reliable, why Unclassified ones are treated as reliable even we don't
know
> for sure?
> 
> > and if we have heuristic results for them, we have to treat
> > them as reliable tests 
> 
> As it is determined to be flaky by Swarm rerun, what's the consideration for a
> flaky test to be treated as reliable one here?
> 
> > and display what we found on SoM. What makes it better is
> > that we now have confidence score, so it should provide some level of
guidance
> > to sheriffs about how trustworthy the result is.
> 

Basically true means Findit thinks the failure is flaky, and false means
everything else.

And for unclassified failures, the results are not necessarily to be wrong, so
we should show the result with confidence score

[chanli, 2016-10-26 06:06:46]

ptal

[lijeffrey, 2016-10-26 22:34:41]

lgtm

[lijeffrey, 2016-10-28 02:32:33]

still lgtm, just 1 nit

https://codereview.chromium.org/2439553002/diff/120001/appengine/findit/water...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2439553002/diff/120001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:118: confidences, cl_build,
cl_first_build):
nit: rename these cl_from_analyzed_build and  cl_from_first_failed_build for
clarity

[chanli, 2016-10-28 02:41:36]

The CQ bit was checked by chanli@chromium.org

[chanli, 2016-10-28 02:41:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
lijeffrey@chromium.org
Link to the patchset: https://codereview.chromium.org/2439553002/#ps140001
(title: "fix nit")

[commit-bot: I haz the power, 2016-10-28 02:41:40]

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

[chanli, 2016-10-28 02:41:46]

https://codereview.chromium.org/2439553002/diff/120001/appengine/findit/water...
File appengine/findit/waterfall/suspected_cl_util.py (right):

https://codereview.chromium.org/2439553002/diff/120001/appengine/findit/water...
appengine/findit/waterfall/suspected_cl_util.py:118: confidences, cl_build,
cl_first_build):
On 2016/10/28 02:32:32, lijeffrey wrote:
> nit: rename these cl_from_analyzed_build and  cl_from_first_failed_build for
> clarity

Done.

[commit-bot: I haz the power, 2016-10-28 02:43:29]

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

[commit-bot: I haz the power, 2016-10-28 02:43:30]

Try jobs failed on following builders:
  Infra Linux Precise 32 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3221fbbdb3643310)
  Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3221fbbd82314d10)

[chanli, 2016-10-28 03:13:29]

The CQ bit was checked by chanli@chromium.org

[chanli, 2016-10-28 03:13:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
lijeffrey@chromium.org
Link to the patchset: https://codereview.chromium.org/2439553002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-28 03:13:37]

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-10-28 04:15:49]

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

[commit-bot: I haz the power, 2016-10-28 04:15:49]

Try jobs failed on following builders:
  Infra Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/32223567607f0310)

[chanli, 2016-10-28 04:18:47]

The CQ bit was checked by chanli@chromium.org

[commit-bot: I haz the power, 2016-10-28 04:18:50]

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-10-28 04:26:47]

Description was changed from

==========
[Findit] Reduce redundant ndb reads by querying necessary entities ahead of time
and share them among failures.

BUG=
==========

to

==========
[Findit] Reduce redundant ndb reads by querying necessary entities ahead of time
and share them among failures.

BUG=

Committed:
https://chromium.googlesource.com/infra/infra/+/0d99c79a624c624b5bd42dfb0a6bc...
==========

[commit-bot: I haz the power, 2016-10-28 04:26:48]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/infra/infra/+/0d99c79a624c624b5bd42dfb0a6bc...