-
Notifications
You must be signed in to change notification settings - Fork 161
Add multi-collector documentation #2298
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
Add multi-collector documentation #2298
Conversation
docs/multiple-collectors.md
Outdated
|
|
||
| ### Master artifacts | ||
|
|
||
| The website will go through all recent master commits, and check if they are done by looking at the `sha` and `status` column in the `benchmark_request` table. |
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.
This is not really what happens right now, because we just load the known SHAs into the benchmark request index.
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.
I've corrected this now 👍
docs/multiple-collectors.md
Outdated
|
|
||
| > The logic for try artifacts can either happen both in cron and in the GH webhook listener (that receives `@rust-timer queue/build` notifications), or only in cron. | ||
| The website will go through all try artifacts in `benchmark_request` that are not yet marked as `completed`. |
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.
In general, the text here is very pseudo-codey and doesn't correspond to how rustc-perf operates. It would be better to describe what the cron job does more literaly, I think.
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.
I feel I've done both, as attempt to explain the CRON more literally. I think this is quite useful, I've moved it to after the explanation of the CRON which feels like it flows a bit better now
…a commit should be queued
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.
I think that it might be better to describe the functionality around what happens in the code in master currently. The HackMD still describes conceptual ideas (like recursive lookup of parents and checking if a job is in a job queue or not), which don't actually happen in the code, so it would be confusing to describe it as such in the document.
So something like:
- When a cron job runs, it scans master commits and releases from the web, if they are not in the benchmark request index, it adds them to the DB.
- Then it tries to finish in-progress request(s).
- Then it build the request queue, and tries to enqueue the first request, if there is nothing in progress.
- During enqueuing, we insert jobs both for the request and its parent; if the parent jobs already existed, nothing new will be inserted.
docs/multiple-collectors.md
Outdated
|
|
||
| - If the request is `waiting for artifacts`, do nothing (sometime later a GH notification will switch the status to `waiting for parent` once the artifacts are ready). | ||
| - If the request is `waiting for parent`: | ||
| - Recursively find a set of **grandparent** master commits that are missing data (by looking at their status in `benchmark_request`). This could happen on the edge switch from `waiting for artifacts` to `waiting for parent` in the GH webhook handler, or it could happen in each cron invocation. |
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.
We don't do this recursive lookup.
docs/multiple-collectors.md
Outdated
| - If the request is `waiting for artifacts`, do nothing (sometime later a GH notification will switch the status to `waiting for parent` once the artifacts are ready). | ||
| - If the request is `waiting for parent`: | ||
| - Recursively find a set of **grandparent** master commits that are missing data (by looking at their status in `benchmark_request`). This could happen on the edge switch from `waiting for artifacts` to `waiting for parent` in the GH webhook handler, or it could happen in each cron invocation. | ||
| - If that set is empty, generate all necessary **parent** jobs and check if they are all completed in the `job_queue`. |
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.
We don't check the parent jobs.
docs/multiple-collectors.md
Outdated
| - If the request's sha is in the benchmark index, nothing happens. | ||
| - If the request is `in_progress`, check [request completion](#Checking-request-completion). | ||
| - If the request is `waiting_for_parent` commit benchmark to be completed, nothing happens. | ||
| - If the request is missing, we will recursively find a set of parent master commits that are missing data (by looking at their status in `benchmark_request`). |
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.
Same here, we don't recursively check for missing parents.
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.
I've removed these two blocks as they don't really add anything, I've added to the key terms and improved the CRON write up; bda5b90
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!
It is effectively a consolidation of the hack.md's