-
Notifications
You must be signed in to change notification settings - Fork 160
Add job benchmark loop #2226
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
base: master
Are you sure you want to change the base?
Add job benchmark loop #2226
Conversation
collector/src/toolchain.rs
Outdated
component, | ||
urls | ||
)) | ||
if !non_404_error { |
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 could be mistaken however this reads slightly oddly; if http is not a not found 404
then we produce an error of sha not found? Then we fall through to an IO error if it was a 404
despite the sha actually not being found.
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.
The variable name wasn't great, renamed it and added a comment.
if !non_404_error { | ||
Err(SysrootDownloadError::SysrootShaNotFound) | ||
} else { | ||
Err(SysrootDownloadError::IO(anyhow!( |
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 can use resp.error_for_status()
so we can get the actual error message too from the response which could be useful for debugging; https://docs.rs/reqwest/latest/reqwest/struct.Response.html#method.error_for_status
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.
error_for_status()
is nice, but the reason why we don't provide more context here is simply because we have up to three (potentially different) errors. Now that we detect 404s explicitly, we could just bail out on the first non-404 error, but I'm a bit worried about backwards compatibility, I don't know if "toolchain not found" is always reported with a 404.
This PR adds the main logic required to execute job benchmarks in the collector.
It handles: