Closed
Bug 1289122
Opened 8 years ago
Closed 8 years ago
Remove coalescing from PGO try jobs
Categories
(Firefox Build System :: Task Configuration, task)
Firefox Build System
Task Configuration
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mtabara, Assigned: mtabara)
References
Details
Attachments
(1 file, 1 obsolete file)
As per https://bugzilla.mozilla.org/show_bug.cgi?id=1274310#c5 we need to unplug coalescing from PGO try jobs.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66824/
Attachment #8774339 -
Flags: review?(dustin)
Assignee | ||
Comment 2•8 years ago
|
||
Sorry for the long delay in pushing this long-overdue patch for review.
Two questions:
1. Not sure how to test this change. Should I build (no tests) across all platforms on try?
2. I've used a regex there instead of direct removal having in mind that we might change the naming convention of the files at some point. In the current form, the patch affects PGO builds on try only. Do we want the same behavior for Opt/Debug Linux32 builds as well? (which are currently using coalescer as well per https://coalesce.mozilla-releng.net/v1/threshold ). If so, I should get rid of that trailing "pgo" in the regex.
Thank you!
Flags: needinfo?(dustin)
Comment 3•8 years ago
|
||
https://reviewboard.mozilla.org/r/66824/#review63604
I really don't care what you do to legacy.py, so this seems fine :)
As for testing, you can just run `mach taskgraph -p parameters.yml target` with `parameters.yml` set up for a try job, and look at the resulting JSON (say, using `jq`). If it generates what you want, you're all set! You can also use 'diff' on the output of that command before and after applying your patch, if you'd like.
Comment 4•8 years ago
|
||
Comment on attachment 8774339 [details]
Bug 1289122 - Remove coalescing from PGO try jobs.
https://reviewboard.mozilla.org/r/66824/#review63606
Attachment #8774339 -
Flags: review?(dustin) → review+
Updated•8 years ago
|
Flags: needinfo?(dustin)
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66974/
Attachment #8774539 -
Flags: review?(dustin)
Assignee | ||
Comment 6•8 years ago
|
||
https://reviewboard.mozilla.org/r/66824/#review63606
Thanks for the info on this. I played a bit with the `mach taskgraph target` command, feeding it parameters.yml from a today's earlier try push and found a small bug in the initial code: removing supersederUrl for Linux32 Opt and Debug builds. I chained supersederUrl to the routes deletion and retested. Everything looks fine in the tests.
@dustin: if this gets green light, could you please land it for me as well? I don't have S3 commit level access.
Thanks again for the help.
Comment 7•8 years ago
|
||
Comment on attachment 8774539 [details]
Bug 1289122 - bugfix to chain together the elements to be removed.
https://reviewboard.mozilla.org/r/66974/#review64218
Looks good. Do you mind squashing these two together and pushing to review again? After that, I can click the autolander button.
Attachment #8774539 -
Flags: review?(dustin) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8774339 [details]
Bug 1289122 - Remove coalescing from PGO try jobs.
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66824/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8774539 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/66974/#review64218
Thanks again! Squashed and pushed. Ready to land.
Comment 10•8 years ago
|
||
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/33f7d8838b33
Remove coalescing from PGO try jobs. r=dustin
Comment 11•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 12•8 years ago
|
||
In bug 1286075, I'm going to change this to not coalesce at all on try (which is, I think, the desired behavior).
Updated•7 years ago
|
Product: TaskCluster → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•