Fix case where resource_name not set in stream error #746
Conversation
There was a problem hiding this comment.
fyi: @chrisstaite-menlo
Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, 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, publish-image, ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @aaronmondal and @zbirenbaum)
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, 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, publish-image, ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @aaronmondal and @zbirenbaum)
nativelink-util/src/proto_stream_utils.rs line 1 at r1 (raw file):
// Copyright 2023-2024 The NativeLink Authors. All rights reserved.
fyi: Sadly the diff didn't help here, but this file is just removed parts of grpc_store.rs merged with write_request_stream_wrapper.rs renamed.
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, 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, publish-image, ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @aaronmondal and @zbirenbaum)
nativelink-util/src/proto_stream_utils.rs line 1 at r1 (raw file):
// Copyright 2023-2024 The NativeLink Authors. All rights reserved.
This link might be helpful:
https://www.diffchecker.com/VifJIlx2/
It's a copy-paste of what the merged files (only added, no modifications) diffed against the this new file.
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, 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, publish-image, ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @aaronmondal)
nativelink-util/src/proto_stream_utils.rs line 1 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
This link might be helpful:
https://www.diffchecker.com/VifJIlx2/It's a copy-paste of what the merged files (only added, no modifications) diffed against the this new file.
Thanks, the diffchecker helps a ton
There was a problem hiding this comment.
Reviewed 8 of 9 files at r1, all commit messages.
Reviewable status: 2 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, 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, publish-image, ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable
nativelink-util/src/proto_stream_utils.rs line 123 at r1 (raw file):
// If we successfully got a message, update our internal state with the // message meta data. let maybe_message = match maybe_message {
nit: Is there some way to make this maybe_message logic (and the let above) more readable?
The RBE protocol specifies that if the first message in a stream has the resource_name set all subsequent messages do not need to have it set. With this patch we now honor that requirement for GrpcStore hotpath. closes: TraceMachina#745
allada
force-pushed
the
fix-resource-name-not-set-in-proto
branch
from
ed68586
to
2674584
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 2 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, ubuntu-20.04 / stable, vale
nativelink-util/src/proto_stream_utils.rs line 123 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
nit: Is there some way to make this
maybe_messagelogic (and theletabove) more readable?
Done. This is about as far as I think we should take it.
The RBE protocol specifies that if the first message in a stream has the resource_name set all subsequent messages do not need to have it set. With this patch we now honor that requirement for GrpcStore hotpath.
closes: #745
This change is