Closed Bug 1289122 Opened 4 years ago Closed 4 years ago
Remove coalescing from PGO try jobs
58 bytes, text/x-review-board-request
As per https://bugzilla.mozilla.org/show_bug.cgi?id=1274310#c5 we need to unplug coalescing from PGO try jobs.
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)
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!
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 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+
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)
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 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+
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/
Attachment #8774539 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/66974/#review64218 Thanks again! Squashed and pushed. Ready to land.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/33f7d8838b33 Remove coalescing from PGO try jobs. r=dustin
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
In bug 1286075, I'm going to change this to not coalesce at all on try (which is, I think, the desired behavior).
You need to log in before you can comment on or make changes to this bug.