[Findit] Rerun if the regression range is different.

[Findit] Rerun if the regression range is different.

Since stacktraces are already grouped at crash side, findit needs not to duplicate this check, so only add regression range check for smart rerun.

BUG=600535
Committed: https://chromium.googlesource.com/infra/infra/+/c958b3053825c8e2e4f3b9a99b31446e826ae22d

[Sharu Jiang, 2016-10-04 20:21:29]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2016-10-04 20:24:52]

Patchset #1 (id:20001) has been deleted

[Sharu Jiang, 2016-10-04 20:30:35]

Description was changed from

==========
[Findit] Rerun if the regression range is different.

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

to

==========
[Findit] Rerun if the regression range is different.

Since stacktraces are already grouped at crash side, findit needs not to
duplicate this check, so only add regression range check for smart rerun.

BUG=600535
==========

[Sharu Jiang, 2016-10-04 20:30:35]

katesonia@chromium.org changed reviewers:
+ inferno@chromium.org, stgao@chromium.org, wrengr@chromium.org

[Sharu Jiang, 2016-10-06 00:31:27]

ping :)

[wrengr, 2016-10-07 17:32:06]

lgtm, with one question/nit

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:114: not analysis.failed):
Does it make sense to check analysis.failed after comparing the
regression_ranges? or should analysis.failed be checked first?

[stgao, 2016-10-07 20:29:35]

lgtm with nits.

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:113: if (analysis and (regression_range
== analysis.regression_range) and
Should we also test regression_range is not empty? Sometimes we don't have a
regression range, right?

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/findit_for_client.py (right):

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit_for_client.py:167: def
GetStacktraceParser(client_id):
This seems unused for now.

[Sharu Jiang, 2016-10-07 22:05:04]

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:113: if (analysis and (regression_range
== analysis.regression_range) and
On 2016/10/07 20:29:35, stgao (slow) wrote:
> Should we also test regression_range is not empty? Sometimes we don't have a
> regression range, right?

For those with empty regressions, we can still find suspected_project and
suspected_components. So we still need to run analyses.

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:114: not analysis.failed):
On 2016/10/07 17:32:06, wrengr wrote:
> Does it make sense to check analysis.failed after comparing the
> regression_ranges? or should analysis.failed be checked first?

Done.

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/findit_for_client.py (right):

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
appengine/findit/crash/findit_for_client.py:167: def
GetStacktraceParser(client_id):
On 2016/10/07 20:29:35, stgao (slow) wrote:
> This seems unused for now.

Oops, should have deleted this.

[Sharu Jiang, 2016-10-07 22:07:45]

Patchset #2 (id:60001) has been deleted

[Sharu Jiang, 2016-10-07 22:08:10]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-10-07 22:08:10]

The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org,
stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2378133004/#ps80001
(title: "Fix nits.")

[commit-bot: I haz the power, 2016-10-07 22:08:12]

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

[stgao, 2016-10-07 22:26:04]

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
File appengine/findit/crash/crash_pipeline.py (right):

https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
appengine/findit/crash/crash_pipeline.py:113: if (analysis and (regression_range
== analysis.regression_range) and
On 2016/10/07 22:05:03, sharu jiang wrote:
> On 2016/10/07 20:29:35, stgao (slow) wrote:
> > Should we also test regression_range is not empty? Sometimes we don't have a
> > regression range, right?
> 
> For those with empty regressions, we can still find suspected_project and
> suspected_components. So we still need to run analyses.

What if both regression range are empty? Do we really want to re-analyze?

[commit-bot: I haz the power, 2016-10-07 22:37:03]

Description was changed from

==========
[Findit] Rerun if the regression range is different.

Since stacktraces are already grouped at crash side, findit needs not to
duplicate this check, so only add regression range check for smart rerun.

BUG=600535
==========

to

==========
[Findit] Rerun if the regression range is different.

Since stacktraces are already grouped at crash side, findit needs not to
duplicate this check, so only add regression range check for smart rerun.

BUG=600535

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

[commit-bot: I haz the power, 2016-10-07 22:37:04]

Committed patchset #2 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/c958b3053825c8e2e4f3b9a99b314...

[Sharu Jiang, 2016-10-07 22:40:24]

On 2016/10/07 22:26:04, stgao (slow) wrote:
>
https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
> File appengine/findit/crash/crash_pipeline.py (right):
> 
>
https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
> appengine/findit/crash/crash_pipeline.py:113: if (analysis and
(regression_range
> == analysis.regression_range) and
> On 2016/10/07 22:05:03, sharu jiang wrote:
> > On 2016/10/07 20:29:35, stgao (slow) wrote:
> > > Should we also test regression_range is not empty? Sometimes we don't have
a
> > > regression range, right?
> > 
> > For those with empty regressions, we can still find suspected_project and
> > suspected_components. So we still need to run analyses.
> 
> What if both regression range are empty? Do we really want to re-analyze?

If both are empty, regression_range == analysis.regression_range would be None
== None, then we won't re-analyze

[stgao, 2016-10-07 22:50:43]

On 2016/10/07 22:40:24, sharu jiang wrote:
> On 2016/10/07 22:26:04, stgao (slow) wrote:
> >
>
https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
> > File appengine/findit/crash/crash_pipeline.py (right):
> > 
> >
>
https://codereview.chromium.org/2378133004/diff/40001/appengine/findit/crash/...
> > appengine/findit/crash/crash_pipeline.py:113: if (analysis and
> (regression_range
> > == analysis.regression_range) and
> > On 2016/10/07 22:05:03, sharu jiang wrote:
> > > On 2016/10/07 20:29:35, stgao (slow) wrote:
> > > > Should we also test regression_range is not empty? Sometimes we don't
have
> a
> > > > regression range, right?
> > > 
> > > For those with empty regressions, we can still find suspected_project and
> > > suspected_components. So we still need to run analyses.
> > 
> > What if both regression range are empty? Do we really want to re-analyze?
> 
> If both are empty, regression_range == analysis.regression_range would be None
> == None, then we won't re-analyze

Ah, you are right.