Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 3013713002: [pinpoint] Calculate distances between Changes.

Created:
1 year, 10 months ago by dtu
Modified:
1 year, 9 months ago
Reviewers:
perezju, shatch, sullivan
CC:
catapult-reviews_chromium.org, perf-dashboard-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

[pinpoint] Calculate distances between Changes. We want to know the distances between the Changes so we can show them at the proper distances on the chart. This lets users know how big the regression range is, how far apart the regressions are, and how far along the Job is. Change.Midpoint() and Commit.Midpoint() already do the legwork of downloading DEPS files and calling out to gitiles_service.CommitRange(), so we can avoid extra API calls by making Midpoint() provide the distance information as well. BUG=catapult:#3869

Patch Set 1 #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -46 lines) Patch
M dashboard/dashboard/pinpoint/models/change/change.py View 6 chunks +21 lines, -17 lines 3 comments Download
M dashboard/dashboard/pinpoint/models/change/change_test.py View 3 chunks +10 lines, -10 lines 1 comment Download
M dashboard/dashboard/pinpoint/models/change/commit.py View 3 chunks +13 lines, -8 lines 2 comments Download
M dashboard/dashboard/pinpoint/models/change/commit_test.py View 2 chunks +6 lines, -4 lines 2 comments Download
M dashboard/dashboard/pinpoint/models/job.py View 5 chunks +36 lines, -7 lines 0 comments Download

Messages

Total messages: 8 (5 generated)
dtu
1 year, 10 months ago (2017-09-15 22:01:41 UTC) #2
perezju
Added a few comments for now; would still like to have a careful pass over ...
1 year, 10 months ago (2017-09-19 15:58:05 UTC) #7
perezju
1 year, 9 months ago (2017-09-20 15:29:41 UTC) #8
https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
File dashboard/dashboard/pinpoint/models/change/change.py (right):

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/change/change.py:76: """Returns a Change
halfway between the two given Changes.
After thinking through this, I think the important invariant that makes this
work (and why you complained about my example on your previous CL that: "These A
and B are only possible if the user explicitly set it that way") is that:

 for each pair (commit_a, commit_b) in zip(change_a.commits, change_b.commits)

 at most one of those pairs has a distance > 1; specifically the pair of commits
for the deepest nested DEP'ed repo which we most recently expanded into; commit
pairs for all other repos should be adjacent to each other (distance == 1).

Does that make sense? Is it correct?

If so, we should probably make that requirement explicit in the code. Should
also help to simplify the logic (I think) and make it easier to understand.

https://codereview.chromium.org/3013713002/diff/1/dashboard/dashboard/pinpoin...
dashboard/dashboard/pinpoint/models/change/change.py:201: deps_b =
commit_b.Deps()
The logic would be clearer if we would raise the error here in case of
mismatching DEPS. So the check for mismatching length above is not needed.

Actually, would it make sense to ignore added or removed DEPS, and just add to
both lists of commits the changed DEPS that both have?

Powered by Google App Engine
This is Rietveld 408576698