[Findit] Datamodel change for triaging at CL level.

[Findit] Datamodel change for triaging at CL level.

BUG=636199
Committed: https://chromium.googlesource.com/infra/infra/+/5f742c0bc9157ab37b82661d307b2068d1d9e1e7

[chanli, 2016-09-08 20:26:27]

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

[chanli, 2016-09-08 20:26:28]

ptal

[lijeffrey, 2016-09-08 22:09:27]

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
File appengine/findit/model/base_cl.py (right):

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/base_cl.py:9: """Represents base information about a
suspected cl."""
nit: it seems this docstring should now be "... information about a cl" since
it's no longer necessarily a suspectedCL?

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
File appengine/findit/model/suspected_cl_status.py (right):

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/suspected_cl_status.py:16: PARTIALLY_CORRECT: 'Partially
Correct',
so does this mean this suspected CL is correct for some builds but not for
others?

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/suspected_cl_status.py:17: PARTIALLY_TRIAGED: 'Partially
triaged'
does this mean the suspected CL has only been triaged for a few builds but not
all? Basically we no longer want automatic matching (if a suspected CL is marked
correct for 1 build, it still only applies to that 1 build)?

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
File appengine/findit/model/wf_culprit.py (right):

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/wf_culprit.py:17: builds =
ndb.JsonProperty(indexed=False)
so we indeed want this in both WfSuspectedCL and in WfCulprit too? Is there a
way to consolidate them?

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
File appengine/findit/model/wf_suspected_cl.py (right):

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:66: # Other possible status are :
suspected_cl_status.CORRECT,
nit: no space before :

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:68: #
suspected_cl_status.PARTIALLY_TRIAGED,
nit: . instead of ,

[chanli, 2016-09-08 22:32:29]

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
File appengine/findit/model/base_cl.py (right):

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/base_cl.py:9: """Represents base information about a
suspected cl."""
On 2016/09/08 22:09:23, lijeffrey wrote:
> nit: it seems this docstring should now be "... information about a cl" since
> it's no longer necessarily a suspectedCL?

I think it's still a suspected cl. changed the module name.

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
File appengine/findit/model/suspected_cl_status.py (right):

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/suspected_cl_status.py:16: PARTIALLY_CORRECT: 'Partially
Correct',
On 2016/09/08 22:09:23, lijeffrey wrote:
> so does this mean this suspected CL is correct for some builds but not for
> others?

Yes.

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/suspected_cl_status.py:17: PARTIALLY_TRIAGED: 'Partially
triaged'
On 2016/09/08 22:09:23, lijeffrey wrote:
> does this mean the suspected CL has only been triaged for a few builds but not
> all? Basically we no longer want automatic matching (if a suspected CL is
marked
> correct for 1 build, it still only applies to that 1 build)?

We want to compare failures before automatic triage. So we will triage builds in
group if they have the same failures.

Please refer to
https://docs.google.com/a/google.com/document/d/1xS2b7Z5RCiFBBp3R-jvTg3zfQuel...

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
File appengine/findit/model/wf_culprit.py (right):

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/wf_culprit.py:17: builds =
ndb.JsonProperty(indexed=False)
On 2016/09/08 22:09:24, lijeffrey wrote:
> so we indeed want this in both WfSuspectedCL and in WfCulprit too? Is there a
> way to consolidate them?

For culprit a list of builds is enough, but we need a dict of information
related to each build in WfSuspectedCL...

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
File appengine/findit/model/wf_suspected_cl.py (right):

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:66: # Other possible status are :
suspected_cl_status.CORRECT,
On 2016/09/08 22:09:24, lijeffrey wrote:
> nit: no space before :

Done.

https://codereview.chromium.org/2320153003/diff/20001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:68: #
suspected_cl_status.PARTIALLY_TRIAGED,
On 2016/09/08 22:09:25, lijeffrey wrote:
> nit: . instead of ,

Done.

[chanli, 2016-09-08 22:34:13]

Add changes

[chanli, 2016-09-12 20:15:06]

On 2016/09/08 22:34:13, chanli wrote:
> Add changes

Add changes.

[stgao, 2016-09-13 17:23:27]

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/common...
File appengine/findit/common/waterfall/failure_type.py (right):

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/common...
appengine/findit/common/waterfall/failure_type.py:9: BOTH = 0x18
What does 'both' mean here? Both compile and test failure?

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
File appengine/findit/model/analysis_approach_type.py (right):

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
appengine/findit/model/analysis_approach_type.py:7: HEURISTIC = 0x01
Do we save these values in datastore? If not, fine to change; if so, should not
change.

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
File appengine/findit/model/base_suspected_cl.py (right):

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
appengine/findit/model/base_suspected_cl.py:12: repo_name =
ndb.StringProperty(indexed=True)
add empty lines in between.

Could have separators "####" for different sections.

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
File appengine/findit/model/wf_suspected_cl.py (right):

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:62: builds =
ndb.JsonProperty(indexed=False)
Do use compressed=True here. This will grow big.

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:76: # Failure type:
failure_type.COMPILE, failure_type.TEST or failure_type.BOTH.
Worth a comment to say a CL could cause compile and test failures in different
build/test configs.

[chanli, 2016-09-13 18:06:30]

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/common...
File appengine/findit/common/waterfall/failure_type.py (right):

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/common...
appengine/findit/common/waterfall/failure_type.py:9: BOTH = 0x18
On 2016/09/13 17:23:27, stgao (slow) wrote:
> What does 'both' mean here? Both compile and test failure?

This type should be really rare. But I remembered you mentioned a case where the
same cl is responsible for a compile failure and another test failures( one of
them was wrong, but still).

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
File appengine/findit/model/analysis_approach_type.py (right):

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
appengine/findit/model/analysis_approach_type.py:7: HEURISTIC = 0x01
On 2016/09/13 17:23:27, stgao (slow) wrote:
> Do we save these values in datastore? If not, fine to change; if so, should
not
> change.

I just checked and they are not in datastore.

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
File appengine/findit/model/base_suspected_cl.py (right):

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
appengine/findit/model/base_suspected_cl.py:12: repo_name =
ndb.StringProperty(indexed=True)
On 2016/09/13 17:23:27, stgao (slow) wrote:
> add empty lines in between.
> 
> Could have separators "####" for different sections.

Done.

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
File appengine/findit/model/wf_suspected_cl.py (right):

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:62: builds =
ndb.JsonProperty(indexed=False)
On 2016/09/13 17:23:27, stgao (slow) wrote:
> Do use compressed=True here. This will grow big.

Done.

https://codereview.chromium.org/2320153003/diff/60001/appengine/findit/model/...
appengine/findit/model/wf_suspected_cl.py:76: # Failure type:
failure_type.COMPILE, failure_type.TEST or failure_type.BOTH.
On 2016/09/13 17:23:27, stgao (slow) wrote:
> Worth a comment to say a CL could cause compile and test failures in different
> build/test configs.

Done.

[stgao, 2016-09-13 18:11:52]

lgtm

[lijeffrey, 2016-09-16 17:15:27]

lgtm

[stgao, 2016-09-21 18:18:10]

still lgtm, but with a nit.

https://codereview.chromium.org/2320153003/diff/100001/appengine/findit/commo...
File appengine/findit/common/waterfall/failure_type.py (right):

https://codereview.chromium.org/2320153003/diff/100001/appengine/findit/commo...
appengine/findit/common/waterfall/failure_type.py:9: BOTH = 0x18
BOTH is not clear here. I think it meant to be COMPILE_AND_TEST?

[chanli, 2016-09-21 21:42:58]

https://codereview.chromium.org/2320153003/diff/100001/appengine/findit/commo...
File appengine/findit/common/waterfall/failure_type.py (right):

https://codereview.chromium.org/2320153003/diff/100001/appengine/findit/commo...
appengine/findit/common/waterfall/failure_type.py:9: BOTH = 0x18
On 2016/09/21 18:18:10, stgao (slow) wrote:
> BOTH is not clear here. I think it meant to be COMPILE_AND_TEST?

Done.

[chanli, 2016-09-23 00:20:52]

Sorry, forgot to update index.yaml....

[stgao, 2016-09-23 06:12:33]

https://codereview.chromium.org/2320153003/diff/160001/appengine/findit/index...
File appengine/findit/index.yaml (right):

https://codereview.chromium.org/2320153003/diff/160001/appengine/findit/index...
appengine/findit/index.yaml:22: - name: approaches
qq: what's the use case to index a list of approaches?

[stgao, 2016-09-23 16:47:23]

https://codereview.chromium.org/2320153003/diff/160001/appengine/findit/index...
File appengine/findit/index.yaml (right):

https://codereview.chromium.org/2320153003/diff/160001/appengine/findit/index...
appengine/findit/index.yaml:22: - name: approaches
On 2016/09/23 06:12:33, stgao (slow) wrote:
> qq: what's the use case to index a list of approaches?

Will the order of items in the list matter? Has the caller code handle this
appropriately? Or we should sort it within WfSuspectedCL before saving?

[chanli, 2016-09-23 16:53:29]

https://codereview.chromium.org/2320153003/diff/160001/appengine/findit/index...
File appengine/findit/index.yaml (right):

https://codereview.chromium.org/2320153003/diff/160001/appengine/findit/index...
appengine/findit/index.yaml:22: - name: approaches
On 2016/09/23 06:12:33, stgao (slow) wrote:
> qq: what's the use case to index a list of approaches?

approaches is repeated property. I need this index to query all cls found by
heuristic or try job.

[stgao, 2016-09-23 16:58:03]

On 2016/09/23 16:47:23, stgao (slow) wrote:
>
https://codereview.chromium.org/2320153003/diff/160001/appengine/findit/index...
> File appengine/findit/index.yaml (right):
> 
>
https://codereview.chromium.org/2320153003/diff/160001/appengine/findit/index...
> appengine/findit/index.yaml:22: - name: approaches
> On 2016/09/23 06:12:33, stgao (slow) wrote:
> > qq: what's the use case to index a list of approaches?
> 
> Will the order of items in the list matter? Has the caller code handle this
> appropriately? Or we should sort it within WfSuspectedCL before saving?

How about my new comment above?

[stgao, 2016-09-23 17:12:04]

Per discussion offline, lgtm.

BTW, we need to be careful to avoid the same status shows up twice in the list.

[chanli, 2016-10-05 17:27:48]

The CQ bit was checked by chanli@chromium.org

[chanli, 2016-10-05 17:27:48]

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/2320153003/#ps160001
(title: "change index.yaml")

[commit-bot: I haz the power, 2016-10-05 17:27:58]

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-05 17:41:58]

Description was changed from

==========
[Findit] Datamodel change for triaging at CL level.

BUG=636199
==========

to

==========
[Findit] Datamodel change for triaging at CL level.

BUG=636199

Committed:
https://chromium.googlesource.com/infra/infra/+/5f742c0bc9157ab37b82661d307b2...
==========

[commit-bot: I haz the power, 2016-10-05 17:41:59]

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