Remove ClientOperationId and move all to OperationId #1214
Conversation
There was a problem hiding this comment.
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.
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.
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.
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.
allada
force-pushed
the
remove-client-operation-id
branch
2 times, most recently
from
231676c to
d27f7f4
Compare
There was a problem hiding this comment.
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
allada
force-pushed
the
remove-client-operation-id
branch
from
d27f7f4 to
bf2f914
Compare
There was a problem hiding this comment.
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