-
Notifications
You must be signed in to change notification settings - Fork 205
Be clearer about what property values workers are missing #2121
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: main
Are you sure you want to change the base?
Be clearer about what property values workers are missing #2121
Conversation
| // No properties required, all workers match | ||
| return self.all_workers.clone(); | ||
| } | ||
|
|
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.
These two got swapped over. In the "you have no workers case" the return value won't change, but it'll be a bit clearer as to why nothing is getting scheduled.
| if full_worker_logging { | ||
| info!( | ||
| "No candidate workers due to a lack of matching {name} = {value:?}" | ||
| "No candidate workers due to a lack of matching '{name}' = {value:?}" |
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.
Otherwise you got things like due to a lack of key os and this more clearly delineates between the key and the message.
| info!("No candidate workers due to a lack of key {name}"); | ||
| info!( | ||
| "No candidate workers due to a lack of key '{name}'. Job asked for {value:?}" | ||
| ); |
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 the main addition, as errors like No candidate workers due to a lack of key InputRootAbsolutePath are hard to fix without knowing what value the jobs are looking for.
e6cbec7 to
adead3b
Compare
adead3b to
dc4eb45
Compare
|
/build-image |
|
Image built and pushed! |
|
/build-image nativelink-worker-init |
|
Image built and pushed! |
Description
#2118 added logging around missing worker properties, but you still have cases where you don't know what property value should actually be set on the workers! This PR improves that.
Type of change
Please delete options that aren't relevant.
How Has This Been Tested?
bazel test //...mostly.Checklist
bazel test //...passes locallygit amendsee some docsThis change is