-
Notifications
You must be signed in to change notification settings - Fork 131
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 ability to prefix worker_id in config #1578
Conversation
WorkerId can now be prefixed with a config. This will cause all internal WorkerIds to be prefixed with this config followed by a generated UUIDv6.
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.
Reviewable status: 0 of 1 LGTMs obtained, and 0 of 20 files reviewed, and pending CI: Web Platform Deployment / ubuntu-24.04, pre-commit-checks, and 1 discussions need to be resolved (waiting on @aaronmondal)
a discussion (no related file):
+@aaronmondal
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.
Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / macos-13, Bazel Dev / macos-14, Cargo Dev / macos-13, Coverage, Installation / macos-13, Installation / macos-14, Local / lre-rs / macos-14, NativeLink.com Cloud / Remote Cache / macos-14, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, docker-compose-compiles-nativelink (22.04), macos-13, windows-2022 / stable, and 1 discussions need to be resolved
nativelink-util/src/action_messages.rs
line 170 at r1 (raw file):
} impl From<String> for WorkerId {
nit: I believe you can imlement From<&str> for WorkerId
so that you don't need to_string
on callsites.
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.
Reviewable status: 1 of 1 LGTMs obtained, and all files reviewed, and 1 discussions need to be resolved
nativelink-util/src/action_messages.rs
line 170 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: I believe you can imlement
From<&str> for WorkerId
so that you don't needto_string
on callsites.
Normally yes, but in this case, I want to consume the string, so I can skip an allocation.
WorkerId can now be prefixed with a config. This will cause all internal WorkerIds to be prefixed with this config followed by a generated UUIDv6.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)