Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in

Issue 3013713002: [pinpoint] Calculate distances between Changes.

2 years, 1 month ago by dtu
2 years ago
perezju, shatch, sullivan
Target Ref:


[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/ View 6 chunks +21 lines, -17 lines 3 comments Download
M dashboard/dashboard/pinpoint/models/change/ View 3 chunks +10 lines, -10 lines 1 comment Download
M dashboard/dashboard/pinpoint/models/change/ View 3 chunks +13 lines, -8 lines 2 comments Download
M dashboard/dashboard/pinpoint/models/change/ View 2 chunks +6 lines, -4 lines 2 comments Download
M dashboard/dashboard/pinpoint/models/ View 5 chunks +36 lines, -7 lines 0 comments Download


Total messages: 8 (5 generated)
2 years, 1 month ago (2017-09-15 22:01:41 UTC) #2
Added a few comments for now; would still like to have a careful pass over ...
2 years, 1 month ago (2017-09-19 15:58:05 UTC) #7
2 years, 1 month ago (2017-09-20 15:29:41 UTC) #8
File dashboard/dashboard/pinpoint/models/change/ (right):
dashboard/dashboard/pinpoint/models/change/ """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.
dashboard/dashboard/pinpoint/models/change/ deps_b =
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