[Findit] Add cracas_crash_analysis

[Findit] Add cracas_crash_analysis

Add CracasCrashAnalysis, the model is identical as FracasCrashAnalysis, so just make a copy to help findit seperate analysis from Fracas and Cracas in database.

BUG=645203
Committed: https://chromium.googlesource.com/infra/infra/+/aaedba6f2626f2493ba5b7a2814bf16ba6806799

[Sharu Jiang, 2016-09-08 21:11:24]

Description was changed from

==========
[Findit] Add cracas_crash_analysis

BUG=645203
==========

to

==========
[Findit] Add cracas_crash_analysis

Add CracasCrashAnalysis, the model is identical as FracasCrashAnalysis, so just
make a copy to help findit seperate analysis from Fracas and Cracas in database.

BUG=645203
==========

[Sharu Jiang, 2016-09-08 21:11:24]

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

[Sharu Jiang, 2016-09-08 21:11:44]

PTAL :)

[Martin Barbella, 2016-09-08 21:15:39]

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

[Martin Barbella, 2016-09-08 21:15:39]

https://codereview.chromium.org/2322133002/diff/20001/appengine/findit/model/...
File appengine/findit/model/crash/cracas_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/20001/appengine/findit/model/...
appengine/findit/model/crash/cracas_crash_analysis.py:13: class
CracasCrashAnalysis(FracasCrashAnalysis):
This doesn't seem clean to me, and I suspect even ClusterFuzz will look very
similar. Could you create a base class for these, and have the rest inherit from
it? Doing so may not even be necessary if we simply include the client in the
model and index on it.

[Sharu Jiang, 2016-09-08 22:18:52]

https://codereview.chromium.org/2322133002/diff/20001/appengine/findit/model/...
File appengine/findit/model/crash/cracas_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/20001/appengine/findit/model/...
appengine/findit/model/crash/cracas_crash_analysis.py:13: class
CracasCrashAnalysis(FracasCrashAnalysis):
On 2016/09/08 21:15:39, Martin Barbella wrote:
> This doesn't seem clean to me, and I suspect even ClusterFuzz will look very
> similar. Could you create a base class for these, and have the rest inherit
from
> it? Doing so may not even be necessary if we simply include the client in the
> model and index on it.

We already have a base class - CrashAnalysis, the inherited FracasCrashAnalysis
will only add historical_metadata and channel fields (data in customized_data),
since CracasCrashAnalysis will also add these two, so I choose make a copy here
(I do need to overwrite one method though, this would be included in the next
patch.), if duplicating code in FracasCrashAnalysis would seem more clear, I can
do that too.

[wrengr, 2016-09-08 22:39:08]

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

[wrengr (wrong one), 2016-09-09 17:58:24]

https://codereview.chromium.org/2322133002/diff/40001/appengine/findit/model/...
File appengine/findit/model/crash/cracas_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/40001/appengine/findit/model/...
appengine/findit/model/crash/cracas_crash_analysis.py:14: class
CracasCrashAnalysis(FracasCrashAnalysis):
Like mbarbella says, this doesn't look clean. Especially given the intent to
deprecate FracasCrashAnalysis in the (presumably near) future, it'd be a lot
better to just factor things out now. There should be some base class that
contains the historic metadata and whatever else is shared, and then both the
FracasCrashAnalysis and CracasCrashAnalysis will be thin subclasses that only
override the method you mentioned. (Whether the shared base class is the same as
CrashAnalysis or different is up to you.)

Classes are cheap. Use them freely to help organize the code, name key concepts,
and keep concerns separate. Even when there's only one small difference between
a subclass and its superclass, if the classes are doing conceptually different
things then make them different classes. The class hierarchy should match the
conceptual ontology; since CracasCrashAnalysis isn't "a type of"
FracasCrashAnalysis, it shouldn't inherit from there.

[Sharu Jiang, 2016-09-09 23:15:54]

Patchset #4 (id:60001) has been deleted

[Sharu Jiang, 2016-09-09 23:15:58]

Patchset #5 (id:100001) has been deleted

[Sharu Jiang, 2016-09-09 23:16:13]

Patchset #4 (id:80001) has been deleted

[Sharu Jiang, 2016-09-09 23:19:43]

https://codereview.chromium.org/2322133002/diff/40001/appengine/findit/model/...
File appengine/findit/model/crash/cracas_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/40001/appengine/findit/model/...
appengine/findit/model/crash/cracas_crash_analysis.py:14: class
CracasCrashAnalysis(FracasCrashAnalysis):
On 2016/09/09 17:58:24, wrengr (wrong one) wrote:
> Like mbarbella says, this doesn't look clean. Especially given the intent to
> deprecate FracasCrashAnalysis in the (presumably near) future, it'd be a lot
> better to just factor things out now. There should be some base class that
> contains the historic metadata and whatever else is shared, and then both the
> FracasCrashAnalysis and CracasCrashAnalysis will be thin subclasses that only
> override the method you mentioned. (Whether the shared base class is the same
as
> CrashAnalysis or different is up to you.)
> 
> Classes are cheap. Use them freely to help organize the code, name key
concepts,
> and keep concerns separate. Even when there's only one small difference
between
> a subclass and its superclass, if the classes are doing conceptually different
> things then make them different classes. The class hierarchy should match the
> conceptual ontology; since CracasCrashAnalysis isn't "a type of"
> FracasCrashAnalysis, it shouldn't inherit from there.

Done.

[wrengr (wrong one), 2016-09-10 00:22:06]

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/chrome_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/chrome_crash_analysis.py:14: """Represents an
analysis of a Chrome crash (Cracas and Fracas)."""
nit: Crash should be capitalized

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/chrome_crash_analysis.py:35: return analysis
It's not part of your diff, but while you're at it this method should just
return the analysis directly, rather than stashing it in a variable first.

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/cracas_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/cracas_crash_analysis.py:12: # TODO(katesonia):
Deprecate FracasCrashAnalysis after switching from Fracas
This todo is no longer relevant (for this file)

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/test/chrome_crash_analysis_test.py (right):

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/test/chrome_crash_analysis_test.py:17: class
_TestChromeCrashAnalysis(ChromeCrashAnalysis):
docstring for what this class is up to?

[stgao, 2016-09-13 17:04:47]

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/fracas_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/fracas_crash_analysis.py:14: """Represents an
analysis of a Chrome crash."""
a Chrome crash on Fracas?

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/test/chrome_crash_analysis_test.py (right):

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/test/chrome_crash_analysis_test.py:17: class
_TestChromeCrashAnalysis(ChromeCrashAnalysis):
The name is a bit confusing. Is it just to subclass ChromeCrashAnalaysis for
testing purpose?

[Sharu Jiang, 2016-09-14 20:46:36]

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/chrome_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/chrome_crash_analysis.py:14: """Represents an
analysis of a Chrome crash (Cracas and Fracas)."""
On 2016/09/10 00:22:06, wrengr (wrong one) wrote:
> nit: Crash should be capitalized

Done.

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/chrome_crash_analysis.py:35: return analysis
On 2016/09/10 00:22:06, wrengr (wrong one) wrote:
> It's not part of your diff, but while you're at it this method should just
> return the analysis directly, rather than stashing it in a variable first.

Done.

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/cracas_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/cracas_crash_analysis.py:12: # TODO(katesonia):
Deprecate FracasCrashAnalysis after switching from Fracas
On 2016/09/10 00:22:06, wrengr (wrong one) wrote:
> This todo is no longer relevant (for this file)

Done.

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/fracas_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/fracas_crash_analysis.py:14: """Represents an
analysis of a Chrome crash."""
On 2016/09/13 17:04:47, stgao (slow) wrote:
> a Chrome crash on Fracas?

Done.

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/test/chrome_crash_analysis_test.py (right):

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/test/chrome_crash_analysis_test.py:17: class
_TestChromeCrashAnalysis(ChromeCrashAnalysis):
On 2016/09/13 17:04:47, stgao (slow) wrote:
> The name is a bit confusing. Is it just to subclass ChromeCrashAnalaysis for
> testing purpose?

Yes, this is only for testing purpose, because the ChromeCrashAnalysis is an
interface.

https://codereview.chromium.org/2322133002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/test/chrome_crash_analysis_test.py:17: class
_TestChromeCrashAnalysis(ChromeCrashAnalysis):
On 2016/09/10 00:22:06, wrengr (wrong one) wrote:
> docstring for what this class is up to?

Done.

[Martin Barbella, 2016-09-19 20:24:36]

LGTM once comment is addressed.

https://codereview.chromium.org/2322133002/diff/160001/appengine/findit/model...
File appengine/findit/model/crash/cracas_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/160001/appengine/findit/model...
appengine/findit/model/crash/cracas_crash_analysis.py:18: return
ndb.Key('CracasCrashAnalysis', hashlib.sha1(
We're using hashlib.sha1(...).hexdigest() in a few places, it seems. Could you
create a helper for this?

Abstracting it a bit should make it more straightforward for new people jumping
into this code, or adding a new analysis type later. They won't need to know
it's a sha1 hash, only that some helper is creating the key we're interested in
and can check the implementation if curious.

[Sharu Jiang, 2016-09-19 22:19:06]

https://codereview.chromium.org/2322133002/diff/160001/appengine/findit/model...
File appengine/findit/model/crash/cracas_crash_analysis.py (right):

https://codereview.chromium.org/2322133002/diff/160001/appengine/findit/model...
appengine/findit/model/crash/cracas_crash_analysis.py:18: return
ndb.Key('CracasCrashAnalysis', hashlib.sha1(
On 2016/09/19 20:24:36, Martin Barbella wrote:
> We're using hashlib.sha1(...).hexdigest() in a few places, it seems. Could you
> create a helper for this?
> 
> Abstracting it a bit should make it more straightforward for new people
jumping
> into this code, or adding a new analysis type later. They won't need to know
> it's a sha1 hash, only that some helper is creating the key we're interested
in
> and can check the implementation if curious.

I moved this to the base class CrashAnalysis, because Fracas/Cracas/Clusterfuzz
analysis should use the same _CreateKey function :)

[stgao, 2016-09-20 23:06:03]

lgtm

[Sharu Jiang, 2016-09-21 19:25:31]

Patchset #8 (id:200001) has been deleted

[Sharu Jiang, 2016-09-21 19:25:49]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-09-21 19:25:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
mbarbella@chromium.org, stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2322133002/#ps220001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-09-21 19:25:52]

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-09-21 20:06:09]

Description was changed from

==========
[Findit] Add cracas_crash_analysis

Add CracasCrashAnalysis, the model is identical as FracasCrashAnalysis, so just
make a copy to help findit seperate analysis from Fracas and Cracas in database.

BUG=645203
==========

to

==========
[Findit] Add cracas_crash_analysis

Add CracasCrashAnalysis, the model is identical as FracasCrashAnalysis, so just
make a copy to help findit seperate analysis from Fracas and Cracas in database.

BUG=645203

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

[commit-bot: I haz the power, 2016-09-21 20:06:10]

Committed patchset #8 (id:220001) as
https://chromium.googlesource.com/infra/infra/+/aaedba6f2626f2493ba5b7a2814bf...