A MapReduce job to add units into the anomaly entity.

A MapReduce job to add units into the anomaly entity.

Also adds the units field to the anomaly definition, and adds it at anomaly creation.

Review-Url: https://codereview.chromium.org/2567723002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/d05d6a1908241734476554abb52557ccebbdc11d

[jessimb, 2016-12-09 19:22:49]

jessimb@chromium.org changed reviewers:
+ eakuefner@chromium.org, sullivan@chromium.org

[jessimb, 2016-12-09 19:22:49]

Annie-
Could you take a look at what I have so far? I'm not sure if I'm on the right
track.

Thanks!

[sullivan, 2016-12-12 15:38:51]

This looks correct except for the places I commented. But you should write a
test in mr_test.py to verify before submitting this CL.

https://codereview.chromium.org/2567723002/diff/20001/dashboard/dashboard/mod...
File dashboard/dashboard/models/anomaly.py (right):

https://codereview.chromium.org/2567723002/diff/20001/dashboard/dashboard/mod...
dashboard/dashboard/models/anomaly.py:59: units =
ndb.TextProperty(indexed=False)
Sorry if I missed this in your other CL. This should be StringProperty, not
TextProperty. It should always be less than 1500 characters. indexed=False is
correct and overrides the default indexed=True, but it's good to have the option
to index this in the future if needed, and that's not available with
TextProperty.

https://codereview.chromium.org/2567723002/diff/20001/dashboard/dashboard/mr.py
File dashboard/dashboard/mr.py (right):

https://codereview.chromium.org/2567723002/diff/20001/dashboard/dashboard/mr....
dashboard/dashboard/mr.py:90: entity.units = entity.test.units
Isn't entity.test a key? Don't you need to do:

test = entity.test.get()
if test:
  entity.units = test.units

[jessimb, 2016-12-12 19:45:47]

I added in two tests and responded to the comments.
PTAL!

https://codereview.chromium.org/2567723002/diff/20001/dashboard/dashboard/mod...
File dashboard/dashboard/models/anomaly.py (right):

https://codereview.chromium.org/2567723002/diff/20001/dashboard/dashboard/mod...
dashboard/dashboard/models/anomaly.py:59: units =
ndb.TextProperty(indexed=False)
On 2016/12/12 at 15:38:51, sullivan wrote:
> Sorry if I missed this in your other CL. This should be StringProperty, not
TextProperty. It should always be less than 1500 characters. indexed=False is
correct and overrides the default indexed=True, but it's good to have the option
to index this in the future if needed, and that's not available with
TextProperty.

Ah, I didn't know the difference, thanks!

https://codereview.chromium.org/2567723002/diff/20001/dashboard/dashboard/mr.py
File dashboard/dashboard/mr.py (right):

https://codereview.chromium.org/2567723002/diff/20001/dashboard/dashboard/mr....
dashboard/dashboard/mr.py:90: entity.units = entity.test.units
On 2016/12/12 at 15:38:51, sullivan wrote:
> Isn't entity.test a key? Don't you need to do:
> 
> test = entity.test.get()
> if test:
>   entity.units = test.units

Makes sense!

[sullivan, 2016-12-12 20:00:18]

lgtm

j

https://codereview.chromium.org/2567723002/diff/40001/dashboard/dashboard/mr_...
File dashboard/dashboard/mr_test.py (right):

https://codereview.chromium.org/2567723002/diff/40001/dashboard/dashboard/mr_...
dashboard/dashboard/mr_test.py:239: self.assertIsNone(anomaly_row.get().units)
Can you also add a testcase for an anomaly with test=None? This is an unusual
case but it could happen due to migrations, deleting data over 6 months old,
etc.

[jessimb, 2016-12-12 20:34:59]

Added in the test. Thanks for the feedback!

https://codereview.chromium.org/2567723002/diff/40001/dashboard/dashboard/mr_...
File dashboard/dashboard/mr_test.py (right):

https://codereview.chromium.org/2567723002/diff/40001/dashboard/dashboard/mr_...
dashboard/dashboard/mr_test.py:239: self.assertIsNone(anomaly_row.get().units)
On 2016/12/12 at 20:00:18, sullivan wrote:
> Can you also add a testcase for an anomaly with test=None? This is an unusual
case but it could happen due to migrations, deleting data over 6 months old,
etc.

Good idea. I had to fix the mr function to work with it - TDD works!

[jessimb, 2016-12-12 20:35:06]

The CQ bit was checked by jessimb@chromium.org

[jessimb, 2016-12-12 20:35:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/2567723002/#ps60001
(title: "added in another test and fixed the mr fn")

[commit-bot: I haz the power, 2016-12-12 20:35:10]

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-12-12 21:29:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481574906591580,
"parent_rev": "228eecdb6f3b59da2daac738f21b013fb2d8bb41", "commit_rev":
"d05d6a1908241734476554abb52557ccebbdc11d"}

[commit-bot: I haz the power, 2016-12-12 21:29:54]

Description was changed from

==========
A MapReduce job to add units into the anomaly entity.

Also adds the units field to the anomaly definition, and adds it at anomaly
creation.
==========

to

==========
A MapReduce job to add units into the anomaly entity.

Also adds the units field to the anomaly definition, and adds it at anomaly
creation.

Review-Url: https://codereview.chromium.org/2567723002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-12-12 21:29:54]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[jessimb, 2016-12-12 22:18:51]

The CQ bit was checked by jessimb@chromium.org

[jessimb, 2016-12-12 22:18:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/2567723002/#ps80001
(title: "Correcting the mapreduce.yaml")

[commit-bot: I haz the power, 2016-12-12 22:18: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-12-12 22:20:07]

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

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

Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)
  Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)