Issue 2396283002: [Findit] Hook up analysis for CQ flakes.

Issue 2396283002: [Findit] Hook up analysis for CQ flakes. (Closed)

Created:
4 years, 2 months ago by stgao

Modified:
4 years, 2 months ago

Reviewers:
chanli, lijeffrey

CC:
chromium-reviews, infra-reviews+infra_chromium.org, Sharu Jiang

Target Ref:
refs/heads/master

Project:
infra

Visibility:
Public.

More Reviews

Description

[Findit] Hook up analysis for CQ flakes. BUG= Committed: https://chromium.googlesource.com/infra/infra/+/1b160dfce79612b34f30d10225f7535dea20e407

Patch Set 2 : add more tests. #

Patch Set 6 : Address comments. #

Patch Set 7 : Fix nit #

Created: 4 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+533 lines, -13 lines)			Patch
M	appengine/findit/common/constants.py	View	1 2 3 4 5	1 chunk	+8 lines, -0 lines	0 comments	Download
M	appengine/findit/model/flake/flake_analysis_request.py	View	1 2 3 4 5	2 chunks	+11 lines, -2 lines	0 comments	Download
M	appengine/findit/model/flake/test/flake_analysis_request_test.py	View		2 chunks	+21 lines, -3 lines	0 comments	Download
M	appengine/findit/waterfall/flake/flake_analysis_service.py	View	1 2 3 4 5 6	2 chunks	+211 lines, -5 lines	0 comments	Download
M	appengine/findit/waterfall/flake/step_mapper.py	View	1 2 3 4	3 chunks	+3 lines, -3 lines	0 comments	Download
A	appengine/findit/waterfall/flake/test/flake_analysis_service_test.py	View	1 2 3 4 5	1 chunk	+279 lines, -0 lines	0 comments	Download

Messages

Total messages: 13 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

lijeffrey

lgtm with nits https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterfall/flake/flake_analysis_service.py File appengine/findit/waterfall/flake/flake_analysis_service.py (right): https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterfall/flake/flake_analysis_service.py#newcode46 appengine/findit/waterfall/flake/flake_analysis_service.py:46: analysis is needed; otherwise 0. nit: ...

4 years, 2 months ago (2016-10-07 20:45:07 UTC) #4

lijeffrey

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterfall/flake/flake_analysis_service.py File appengine/findit/waterfall/flake/flake_analysis_service.py (right): https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterfall/flake/flake_analysis_service.py#newcode162 appengine/findit/waterfall/flake/flake_analysis_service.py:162: 'chromium-try-flakes@appspot.gserviceaccount.com', does findit-for-me@appspot.gserviceaccount.com count as admin too?

4 years, 2 months ago (2016-10-07 21:22:48 UTC) #5

chanli

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterfall/flake/flake_analysis_service.py File appengine/findit/waterfall/flake/flake_analysis_service.py (right): https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterfall/flake/flake_analysis_service.py#newcode37 appengine/findit/waterfall/flake/flake_analysis_service.py:37: def _NeedNewAnalysis(request): Nit: This kind of name indicates a ...

4 years, 2 months ago (2016-10-07 21:40:37 UTC) #6

stgao

comments addressed. https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterfall/flake/flake_analysis_service.py File appengine/findit/waterfall/flake/flake_analysis_service.py (right): https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterfall/flake/flake_analysis_service.py#newcode37 appengine/findit/waterfall/flake/flake_analysis_service.py:37: def _NeedNewAnalysis(request): On 2016/10/07 21:40:37, chanli wrote: ...

4 years, 2 months ago (2016-10-07 23:07:06 UTC) #7

comments addressed.

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
File appengine/findit/waterfall/flake/flake_analysis_service.py (right):

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:37: def
_NeedNewAnalysis(request):
On 2016/10/07 21:40:37, chanli wrote:
> Nit: This kind of name indicates a True/False return value, could you change
the
> function name to be more clear?

Renamed to CheckForNewAnalyais. Hope it is better, although still not ideal.

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:46: analysis is
needed; otherwise 0.
On 2016/10/07 20:45:07, lijeffrey wrote:
> nit: space before (. Same with build_step

Done.

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:67:
supported_build_step.analyzed = True  # This step will be analyzed.
On 2016/10/07 21:40:37, chanli wrote:
> Based on flake_analysis_request.py, analyzed 'Indicate whether the flake on
this
> configuration is analyzed.'? I thought it means this step has been analyzed
> rather than will be analyzed?

Good point.
Renamed to "scheduled".

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:68: # For unsupported
or step-level flakes, still save them for monitoring.
On 2016/10/07 20:45:07, lijeffrey wrote:
> nit: add 1 empty line before the comment

Done.

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:88: for step in
request.build_steps:
On 2016/10/07 20:45:07, lijeffrey wrote:
> is it possible to move this for loop to a separate function that returns
> existing_step? In general it looks like this else branch is a bit long

I just moved the else branch to another function.

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:143: return
previous_request.version_number, supported_build_step
On 2016/10/07 21:40:37, chanli wrote:
> Just to be clear:
> If there is not previous analysis, or previous analysis has a different bug
id,
> save a new request; If there is a previous analysis, and it has the same bug
id,
> or only one of the request has bug_id, think it as the same request and use
> another configuration (or use the same if previous is too old) and use the
> previous request?
> 
> What if later a request comes in with the same bug_id as the previous request?

Assume only one bug is files for the same flake. And if a new bug is filed, that
means the old bug was fixed.

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:161:
whitelisted_accounts = [
On 2016/10/07 21:40:36, chanli wrote:
> Move this to constant for now?

Done.

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:162:
'chromium-try-flakes@appspot.gserviceaccount.com',
On 2016/10/07 21:22:47, lijeffrey wrote:
> does mailto:findit-for-me@appspot.gserviceaccount.com count as admin too?

No. Admin means admin of the project.

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:166: if not
user_email or not (
On 2016/10/07 20:45:07, lijeffrey wrote:
> nit: I think this should be a separate function _IsUserAuthorized(user_email)
or
> similar

Done.

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:195: return True
On 2016/10/07 21:40:37, chanli wrote:
> Shouldn't return an instance of MasterFlakeAnalysis? According to docstring...

Updated docstring.

https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
appengine/findit/waterfall/flake/flake_analysis_service.py:201: return False
On 2016/10/07 21:40:37, chanli wrote:
> Return None?

Acknowledged.

chanli

On 2016/10/07 23:07:06, stgao (slow) wrote: > comments addressed. > > https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterfall/flake/flake_analysis_service.py > File appengine/findit/waterfall/flake/flake_analysis_service.py ...

4 years, 2 months ago (2016-10-07 23:11:20 UTC) #8

On 2016/10/07 23:07:06, stgao (slow) wrote:
> comments addressed.
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> File appengine/findit/waterfall/flake/flake_analysis_service.py (right):
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:37: def
> _NeedNewAnalysis(request):
> On 2016/10/07 21:40:37, chanli wrote:
> > Nit: This kind of name indicates a True/False return value, could you change
> the
> > function name to be more clear?
> 
> Renamed to CheckForNewAnalyais. Hope it is better, although still not ideal.
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:46: analysis is
> needed; otherwise 0.
> On 2016/10/07 20:45:07, lijeffrey wrote:
> > nit: space before (. Same with build_step
> 
> Done.
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:67:
> supported_build_step.analyzed = True  # This step will be analyzed.
> On 2016/10/07 21:40:37, chanli wrote:
> > Based on flake_analysis_request.py, analyzed 'Indicate whether the flake on
> this
> > configuration is analyzed.'? I thought it means this step has been analyzed
> > rather than will be analyzed?
> 
> Good point.
> Renamed to "scheduled".
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:68: # For
unsupported
> or step-level flakes, still save them for monitoring.
> On 2016/10/07 20:45:07, lijeffrey wrote:
> > nit: add 1 empty line before the comment
> 
> Done.
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:88: for step in
> request.build_steps:
> On 2016/10/07 20:45:07, lijeffrey wrote:
> > is it possible to move this for loop to a separate function that returns
> > existing_step? In general it looks like this else branch is a bit long
> 
> I just moved the else branch to another function.
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:143: return
> previous_request.version_number, supported_build_step
> On 2016/10/07 21:40:37, chanli wrote:
> > Just to be clear:
> > If there is not previous analysis, or previous analysis has a different bug
> id,
> > save a new request; If there is a previous analysis, and it has the same bug
> id,
> > or only one of the request has bug_id, think it as the same request and use
> > another configuration (or use the same if previous is too old) and use the
> > previous request?
> > 
> > What if later a request comes in with the same bug_id as the previous
request?
> 
> Assume only one bug is files for the same flake. And if a new bug is filed,
that
> means the old bug was fixed.
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:161:
> whitelisted_accounts = [
> On 2016/10/07 21:40:36, chanli wrote:
> > Move this to constant for now?
> 
> Done.
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:162:
> mailto:,
> On 2016/10/07 21:22:47, lijeffrey wrote:
> > does mailto:findit-for-me@appspot.gserviceaccount.com count as admin too?
> 
> No. Admin means admin of the project.
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:166: if not
> user_email or not (
> On 2016/10/07 20:45:07, lijeffrey wrote:
> > nit: I think this should be a separate function
_IsUserAuthorized(user_email)
> or
> > similar
> 
> Done.
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:195: return True
> On 2016/10/07 21:40:37, chanli wrote:
> > Shouldn't return an instance of MasterFlakeAnalysis? According to
docstring...
> 
> Updated docstring.
> 
>
https://codereview.chromium.org/2396283002/diff/80001/appengine/findit/waterf...
> appengine/findit/waterfall/flake/flake_analysis_service.py:201: return False
> On 2016/10/07 21:40:37, chanli wrote:
> > Return None?
> 
> Acknowledged.

lgtm

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-07 23:12:53 UTC) #11

commit-bot: I haz the power

4 years, 2 months ago (2016-10-07 23:34:14 UTC) #13

Message was sent while issue was closed.

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/infra/infra/+/1b160dfce79612b34f30d10225f75...

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages