replacing the NullCulprit class by None

replacing the NullCulprit class by None

BUG=659359
TBR=stgao@chromium.org

Committed: https://chromium.googlesource.com/infra/infra/+/85d7a301dd67ee340fb722f096276993f40636de

[wrengr, 2016-10-28 18:37:01]

Description was changed from

==========
replacing the NullCulprit class by None

BUG=659359
R=stgao@chromium.com, katesonia@chromium.com, mbarbella@chromium.com
CC=aarya@google.com
==========

to

==========
replacing the NullCulprit class by None

BUG=659359
==========

[Sharu Jiang, 2016-10-28 20:34:12]

katesonia@chromium.org changed reviewers:
+ katesonia@chromium.org

[Sharu Jiang, 2016-10-28 20:34:13]

https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
appengine/findit/crash/findit.py:279: return None
It's ok to return None here, but you need to update the related part in
CrashAnalysisPipeline in the crash_pipeline.py.

And in this case, you still need to push
{'found': False} to clients and update CrashAnalysis model in data store using 
{'found_suspects': False,
 'found_project': False,
 'found_components': False,
 'has_regression_range': False,
 'solution': None}

[wrengr, 2016-10-28 21:11:20]

https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
appengine/findit/crash/findit.py:279: return None
On 2016/10/28 20:34:12, sharu jiang wrote:
> It's ok to return None here, but you need to update the related part in
> CrashAnalysisPipeline in the crash_pipeline.py.
> 
> And in this case, you still need to push
> {'found': False} to clients and update CrashAnalysis model in data store using

> {'found_suspects': False,
>  'found_project': False,
>  'found_components': False,
>  'has_regression_range': False,
>  'solution': None}

Indeed, the goal of this CL is to allow returning None here. Shouldn't the
'solution' be the algorithm that was tried and failed to find a culprit, as you
said in the other CL?

[wrengr, 2016-10-28 21:12:42]

On 2016/10/28 21:11:20, wrengr wrote:
>
https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
> File appengine/findit/crash/findit.py (right):
> 
>
https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
> appengine/findit/crash/findit.py:279: return None
> On 2016/10/28 20:34:12, sharu jiang wrote:
> > It's ok to return None here, but you need to update the related part in
> > CrashAnalysisPipeline in the crash_pipeline.py.
> > 
> > And in this case, you still need to push
> > {'found': False} to clients and update CrashAnalysis model in data store
using
> 
> > {'found_suspects': False,
> >  'found_project': False,
> >  'found_components': False,
> >  'has_regression_range': False,
> >  'solution': None}
> 
> Indeed, the goal of this CL is to allow returning None here. Shouldn't the
> 'solution' be the algorithm that was tried and failed to find a culprit, as
you
> said in the other CL?

Also, I pushed a new version of the CL which updates CrashAnalysisPipeline to
handle None (and adds a unittest to make sure).

[Sharu Jiang, 2016-10-28 23:40:58]

lgtm

https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
File appengine/findit/crash/findit.py (right):

https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
appengine/findit/crash/findit.py:279: return None
On 2016/10/28 21:11:19, wrengr wrote:
> On 2016/10/28 20:34:12, sharu jiang wrote:
> > It's ok to return None here, but you need to update the related part in
> > CrashAnalysisPipeline in the crash_pipeline.py.
> > 
> > And in this case, you still need to push
> > {'found': False} to clients and update CrashAnalysis model in data store
using
> 
> > {'found_suspects': False,
> >  'found_project': False,
> >  'found_components': False,
> >  'has_regression_range': False,
> >  'solution': None}
> 
> Indeed, the goal of this CL is to allow returning None here. Shouldn't the
> 'solution' be the algorithm that was tried and failed to find a culprit, as
you
> said in the other CL?

Yes, the algorithm is a better name, however when you update the CrashAnalysis
model, it should be 'solution', we can change that field name, however there
would be a legacy problem... though it won't have much impact right now.

[wrengr, 2016-10-30 00:16:13]

On 2016/10/28 23:40:58, sharu jiang wrote:
> lgtm
> 
>
https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
> File appengine/findit/crash/findit.py (right):
> 
>
https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
> appengine/findit/crash/findit.py:279: return None
> On 2016/10/28 21:11:19, wrengr wrote:
> > On 2016/10/28 20:34:12, sharu jiang wrote:
> > > It's ok to return None here, but you need to update the related part in
> > > CrashAnalysisPipeline in the crash_pipeline.py.
> > > 
> > > And in this case, you still need to push
> > > {'found': False} to clients and update CrashAnalysis model in data store
> using
> > 
> > > {'found_suspects': False,
> > >  'found_project': False,
> > >  'found_components': False,
> > >  'has_regression_range': False,
> > >  'solution': None}
> > 
> > Indeed, the goal of this CL is to allow returning None here. Shouldn't the
> > 'solution' be the algorithm that was tried and failed to find a culprit, as
> you
> > said in the other CL?
> 
> Yes, the algorithm is a better name, however when you update the CrashAnalysis
> model, it should be 'solution', we can change that field name, however there
> would be a legacy problem... though it won't have much impact right now.

No no, my point was that the field of the ndb.Model should be filled in with the
algorithm used (rather than with None)

[Sharu Jiang, 2016-10-30 21:11:04]

On 2016/10/30 00:16:13, wrengr wrote:
> On 2016/10/28 23:40:58, sharu jiang wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
> > File appengine/findit/crash/findit.py (right):
> > 
> >
>
https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
> > appengine/findit/crash/findit.py:279: return None
> > On 2016/10/28 21:11:19, wrengr wrote:
> > > On 2016/10/28 20:34:12, sharu jiang wrote:
> > > > It's ok to return None here, but you need to update the related part in
> > > > CrashAnalysisPipeline in the crash_pipeline.py.
> > > > 
> > > > And in this case, you still need to push
> > > > {'found': False} to clients and update CrashAnalysis model in data store
> > using
> > > 
> > > > {'found_suspects': False,
> > > >  'found_project': False,
> > > >  'found_components': False,
> > > >  'has_regression_range': False,
> > > >  'solution': None}
> > > 
> > > Indeed, the goal of this CL is to allow returning None here. Shouldn't the
> > > 'solution' be the algorithm that was tried and failed to find a culprit,
as
> > you
> > > said in the other CL?
> > 
> > Yes, the algorithm is a better name, however when you update the
CrashAnalysis
> > model, it should be 'solution', we can change that field name, however there
> > would be a legacy problem... though it won't have much impact right now.
> 
> No no, my point was that the field of the ndb.Model should be filled in with
the
> algorithm used (rather than with None)

Ah, that sounds good.

[wrengr, 2016-10-31 16:58:45]

Description was changed from

==========
replacing the NullCulprit class by None

BUG=659359
==========

to

==========
replacing the NullCulprit class by None

BUG=659359
TBR=stgao@chromium.com
==========

[wrengr, 2016-10-31 16:58:45]

wrengr@chromium.org changed reviewers:
+ mbarbella@chromium.org
- katesonia@chromium.com, mbarbella@chromium.com, stgao@chromium.com

[wrengr, 2016-10-31 16:58:50]

The CQ bit was checked by wrengr@chromium.org

[commit-bot: I haz the power, 2016-10-31 16:58:59]

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-31 16:59:00]

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

[commit-bot: I haz the power, 2016-10-31 16:59:01]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-infra-committers".
Note that this has nothing to do with OWNERS files.

[wrengr, 2016-10-31 17:05:45]

On 2016/10/30 21:11:04, sharu jiang wrote:
> On 2016/10/30 00:16:13, wrengr wrote:
> > On 2016/10/28 23:40:58, sharu jiang wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
> > > File appengine/findit/crash/findit.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2457153002/diff/1/appengine/findit/crash/find...
> > > appengine/findit/crash/findit.py:279: return None
> > > On 2016/10/28 21:11:19, wrengr wrote:
> > > > On 2016/10/28 20:34:12, sharu jiang wrote:
> > > > > It's ok to return None here, but you need to update the related part
in
> > > > > CrashAnalysisPipeline in the crash_pipeline.py.
> > > > > 
> > > > > And in this case, you still need to push
> > > > > {'found': False} to clients and update CrashAnalysis model in data
store
> > > using
> > > > 
> > > > > {'found_suspects': False,
> > > > >  'found_project': False,
> > > > >  'found_components': False,
> > > > >  'has_regression_range': False,
> > > > >  'solution': None}
> > > > 
> > > > Indeed, the goal of this CL is to allow returning None here. Shouldn't
the
> > > > 'solution' be the algorithm that was tried and failed to find a culprit,
> as
> > > you
> > > > said in the other CL?
> > > 
> > > Yes, the algorithm is a better name, however when you update the
> CrashAnalysis
> > > model, it should be 'solution', we can change that field name, however
there
> > > would be a legacy problem... though it won't have much impact right now.
> > 
> > No no, my point was that the field of the ndb.Model should be filled in with
> the
> > algorithm used (rather than with None)
> 
> Ah, that sounds good.

Saved this todo as http://crbug.com/660887

[stgao, 2016-10-31 17:10:03]

stgao@chromium.org changed reviewers:
+ stgao@chromium.org

[stgao, 2016-10-31 17:10:04]

lgtm

[wrengr, 2016-10-31 17:13:18]

Description was changed from

==========
replacing the NullCulprit class by None

BUG=659359
TBR=stgao@chromium.com
==========

to

==========
replacing the NullCulprit class by None

BUG=659359
TBR=stgao@chromium.org
==========

[wrengr, 2016-10-31 17:13:26]

The CQ bit was checked by wrengr@chromium.org

[commit-bot: I haz the power, 2016-10-31 17:13:32]

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-31 17:15:21]

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

[commit-bot: I haz the power, 2016-10-31 17:15:22]

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

[wrengr, 2016-10-31 17:23:02]

The CQ bit was checked by wrengr@chromium.org

[wrengr, 2016-10-31 17:23:02]

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

[commit-bot: I haz the power, 2016-10-31 17:23:13]

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-31 17:37:31]

Description was changed from

==========
replacing the NullCulprit class by None

BUG=659359
TBR=stgao@chromium.org
==========

to

==========
replacing the NullCulprit class by None

BUG=659359
TBR=stgao@chromium.org

Committed:
https://chromium.googlesource.com/infra/infra/+/85d7a301dd67ee340fb722f096276...
==========

[commit-bot: I haz the power, 2016-10-31 17:37:32]

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