-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: use new batched queries in task replacement (bug 1840830) #496
Conversation
6a7f2a0
to
a8f5f09
Compare
I have no idea what's up with those task failures, never seen that before.. |
Wow this decision task generated quite a messed up graph. Things end up using the index-task docker image instead of whatever docker image they wanted. full-task-graph.json looks sane, but task-graph.json has PebYGdYSQgu0_iTKTCg_fQ as dependency where it shouldn't be. |
I think I didn't have this problem when I hadn't yet changed the tests (to check if the local failures were the same). Does that give any hints as to what I should do @jcristau ? |
3e84498
to
74d994f
Compare
9b4dce5
to
16c441e
Compare
Tests are fixed, I hadn't fixed a problem in both places it was and the result was confusing to me. Everything seems good now. |
Tests are fixed, I hadn't fixed a problem in both places it was and the result was confusing to me. Everything seems good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looking good! This needs a bit of refactoring though
16c441e
to
b079932
Compare
Thanks for the quick review and the very pertinent comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks correct. I have one minor refactor request, then I think this can merge. I'll handle the merge + new release after the next iteration.
b079932
to
6434015
Compare
Batching queries to get from task index to status reduces the number of (sometimes trans-continental) queries from 2*900+ to ~2. This reduces the time spent replacing tasks from 20% to 75% depending on the use-case. The 20% improvement in wall time was observed when running `mach taskgraph morphed` in a CI worker, while the 75% improvement was observed in a developer machine in France running `mach taskgraph full`. More information in taskcluster/taskcluster-rfcs#189.
@ahal Thanks, all of your comments should be addressed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, lgtm!
Batching queries to get from task index to status reduces the number of (sometimes trans-continental) queries from 2*900+ to ~2. This reduces the time spent replacing tasks from 20% to 75% depending on the use-case.
The 20% improvement in wall time was observed when running
mach taskgraph morphed
in a CI worker, while the 75% improvement was observed in a developer machine in France runningmach taskgraph full
.More information in taskcluster/taskcluster-rfcs#189.