[Findit] Fixing analysis mismatch in check flake

[Findit] Fixing analysis mismatch in check flake

BUG=666109
Committed: https://chromium.googlesource.com/infra/infra/+/edad87a65b51aaf0fed58b82007dfabf366f9100

[lijeffrey, 2016-11-18 00:15:24]

Description was changed from

==========
[Findit] Fixing analysis mismatch in check flake

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

to

==========
[Findit] Fixing analysis mismatch in check flake

BUG=666109
==========

[lijeffrey, 2016-11-18 00:15:24]

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

[lijeffrey, 2016-11-18 00:15:48]

ptal

[stgao, 2016-11-18 17:33:42]

lgtm with nits.

https://codereview.chromium.org/2510223003/diff/1/appengine/findit/model/flak...
File appengine/findit/model/flake/flake_analysis_request.py (right):

https://codereview.chromium.org/2510223003/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/flake_analysis_request.py:151: analysis =
analysis_key.get()
Based on the key, we could derive master/builder name, builder number, etc.
I guess the ndb read here could be avoided.

[chanli, 2016-11-18 19:15:28]

lgtm

https://codereview.chromium.org/2510223003/diff/1/appengine/findit/model/flak...
File appengine/findit/model/flake/flake_analysis_request.py (right):

https://codereview.chromium.org/2510223003/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/flake_analysis_request.py:155: return None
If one analysis is deleted, I agree we should log the error, but do we need to
return here? Maybe we'll find another analysis? Although after the change
suggested by Shuotao, this will not be necessary

[lijeffrey, 2016-11-18 20:47:33]

Comments addressed, mind another quick glance over?

https://codereview.chromium.org/2510223003/diff/1/appengine/findit/model/flak...
File appengine/findit/model/flake/flake_analysis_request.py (right):

https://codereview.chromium.org/2510223003/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/flake_analysis_request.py:151: analysis =
analysis_key.get()
On 2016/11/18 17:33:42, stgao (slow on Monday) wrote:
> Based on the key, we could derive master/builder name, builder number, etc.
> I guess the ndb read here could be avoided.

Done.

https://codereview.chromium.org/2510223003/diff/1/appengine/findit/model/flak...
appengine/findit/model/flake/flake_analysis_request.py:155: return None
On 2016/11/18 19:15:28, chanli wrote:
> If one analysis is deleted, I agree we should log the error, but do we need to
> return here? Maybe we'll find another analysis? Although after the change
> suggested by Shuotao, this will not be necessary

Done.

[chanli, 2016-11-18 21:02:46]

https://codereview.chromium.org/2510223003/diff/20001/appengine/findit/model/...
File appengine/findit/model/flake/flake_analysis_request.py (right):

https://codereview.chromium.org/2510223003/diff/20001/appengine/findit/model/...
appengine/findit/model/flake/flake_analysis_request.py:146: if not
normalized_master_name or not normalized_builder_name:
Essentially _GetNormalizedConfigurationNames will return the same master_name
and builder_name as the parameters if match found, right? So this function seems
unnecessary to me? we can just compare them with analysis's configuration,
right?

[stgao, 2016-11-18 21:03:00]

one more nit.

https://codereview.chromium.org/2510223003/diff/20001/appengine/findit/model/...
File appengine/findit/model/flake/flake_analysis_request.py (right):

https://codereview.chromium.org/2510223003/diff/20001/appengine/findit/model/...
appengine/findit/model/flake/flake_analysis_request.py:150: components =
analysis_key.pairs()[0][1].split('/')
How about moving this logic to the MasterFlakeAnalysis class?

[lijeffrey, 2016-11-19 01:00:15]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-11-19 01:00:15]

The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org,
stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2510223003/#ps40001
(title: "Fixing nit")

[commit-bot: I haz the power, 2016-11-19 01:00:17]

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

[lijeffrey, 2016-11-19 01:00:25]

https://codereview.chromium.org/2510223003/diff/20001/appengine/findit/model/...
File appengine/findit/model/flake/flake_analysis_request.py (right):

https://codereview.chromium.org/2510223003/diff/20001/appengine/findit/model/...
appengine/findit/model/flake/flake_analysis_request.py:146: if not
normalized_master_name or not normalized_builder_name:
On 2016/11/18 21:02:46, chanli wrote:
> Essentially _GetNormalizedConfigurationNames will return the same master_name
> and builder_name as the parameters if match found, right? So this function
seems
> unnecessary to me? we can just compare them with analysis's configuration,
> right?

Not necessarily, build_step.master_name might != build_step.wf_master_name if
original master_name is from CQ. master_name is assumed to be from the
originally requested source, but will also save the equivalent wf_master_name
too to ensure we find the correct match

https://codereview.chromium.org/2510223003/diff/20001/appengine/findit/model/...
appengine/findit/model/flake/flake_analysis_request.py:150: components =
analysis_key.pairs()[0][1].split('/')
On 2016/11/18 21:03:00, stgao (slow on Monday) wrote:
> How about moving this logic to the MasterFlakeAnalysis class?

Done.

[commit-bot: I haz the power, 2016-11-19 01:02:01]

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

[commit-bot: I haz the power, 2016-11-19 01:02:01]

Try jobs failed on following builders:
  Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3292ead194653710)
  Infra Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3292ead0d2b4bc10)
  Infra Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3292ead0cf339310)

[lijeffrey, 2016-11-19 01:03:15]

The CQ bit was checked by lijeffrey@chromium.org

[commit-bot: I haz the power, 2016-11-19 01:03:21]

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-19 01:05:30]

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

[commit-bot: I haz the power, 2016-11-19 01:05:31]

Try jobs failed on following builders:
  Infra Linux Precise 32 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3292eda2fb582b10)
  Infra Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3292eda0fa15b610)

[lijeffrey, 2016-11-19 01:27:13]

The CQ bit was checked by lijeffrey@chromium.org

[commit-bot: I haz the power, 2016-11-19 01:27:21]

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-19 01:29:11]

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

[commit-bot: I haz the power, 2016-11-19 01:29:12]

Try jobs failed on following builders:
  Infra Linux Precise 32 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/329303965a0ed710)
  Infra Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/329303957db59f10)
  Infra Mac Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/329303974df53a10)
  Infra Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/329303970506f710)

[lijeffrey, 2016-11-19 01:39:46]

The CQ bit was checked by lijeffrey@chromium.org

[lijeffrey, 2016-11-19 01:39:46]

The patchset sent to the CQ was uploaded after l-g-t-m from chanli@chromium.org,
stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2510223003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-19 01:39:57]

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-19 01:55:15]

Description was changed from

==========
[Findit] Fixing analysis mismatch in check flake

BUG=666109
==========

to

==========
[Findit] Fixing analysis mismatch in check flake

BUG=666109

Committed:
https://chromium.googlesource.com/infra/infra/+/edad87a65b51aaf0fed58b82007df...
==========

[commit-bot: I haz the power, 2016-11-19 01:55:16]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/edad87a65b51aaf0fed58b82007df...