-
Notifications
You must be signed in to change notification settings - Fork 104
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
Remove ClientOperationId and move all to OperationId #1214
Conversation
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 26 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable (waiting on @zbirenbaum)
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 26 of 26 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.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 56 at r1 (raw file):
} impl OperationId {
Removing unique qualifier from OperationId makes some things much more difficult. What was the rationale for this?
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 26 of 26 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), macos-13, windows-2022 / stable, and 1 discussions need to be resolved (waiting on @zbirenbaum)
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 all files reviewed, and 1 discussions need to be resolved (waiting on @zbirenbaum)
nativelink-util/src/action_messages.rs
line 56 at r1 (raw file):
Previously, zbirenbaum (Zach Birenbaum) wrote…
Removing unique qualifier from OperationId makes some things much more difficult. What was the rationale for this?
This is incompatible with other similar systems. We can't assume data is encoded into the string and instead should be passing the metadata needed.
231676c
to
d27f7f4
Compare
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 3 of 3 files at r2, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and all files reviewed, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), and 1 discussions need to be resolved (waiting on @zbirenbaum)
This is in prep to support generic layering of workers & schedulers. We can no longer know if the source is a client or just an up-stream-component, so we need to fall back to just using OperationId. towards TraceMachina#1213
d27f7f4
to
bf2f914
Compare
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 pending CI: Analyze (javascript-typescript), Publish image, asan / ubuntu-22.04, integration-tests (20.04), pre-commit-checks, ubuntu-22.04
Minor cosmetic fix to reflect changes in TraceMachina#1214
This is in prep to support generic layering of workers & schedulers. We can no longer know if the source is a client or just an up-stream-component, so we need to fall back to just using OperationId.
towards #1213
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)