Chunker: Always seek on the uncompressed stream.#15669
Closed
benjaminp wants to merge 1 commit intobazelbuild:masterfrom
Closed
Chunker: Always seek on the uncompressed stream.#15669benjaminp wants to merge 1 commit intobazelbuild:masterfrom
benjaminp wants to merge 1 commit intobazelbuild:masterfrom
Conversation
The `WriteRequest.write_offset` field has bizarre semantics during compressed uploads as documented in the remote API protos: https://github.com/bazelbuild/remote-apis/blob/3b4b6402103539d66fcdd1a4d945f660742665ca/build/bazel/remote/execution/v2/remote_execution.proto#L241-L248 In particular, the write offset of the first `WriteRequest` refers to the offset in the uncompressed source. This change ensures we always seek the uncompressed stream to the correct offset when starting an upload call. The old code could fail to resume compressed uploads under some conditions. The `progressiveCompressedUploadShouldWork` test purported to exercise this situation. The test, however, contained the same logic error as the code under test.
Contributor
|
Would this fully fix #14654 then? Might be worth an inclusion in a 5.3 if one gets made. |
coeuvre
approved these changes
Jun 14, 2022
Contributor
|
@bazel-io flag |
|
@bazel-io fork 5.3.0 |
|
Hello @benjaminp, I am trying to cherry pick these changes to release-5.3.0 but presubmit checks are failing. Could you please help me in cherry picking these changes with appropriate commits. Thanks! |
ckolli5
added a commit
that referenced
this pull request
Jul 7, 2022
* Chunker: Always seek on the uncompressed stream. The `WriteRequest.write_offset` field has bizarre semantics during compressed uploads as documented in the remote API protos: https://github.com/bazelbuild/remote-apis/blob/3b4b6402103539d66fcdd1a4d945f660742665ca/build/bazel/remote/execution/v2/remote_execution.proto#L241-L248 In particular, the write offset of the first `WriteRequest` refers to the offset in the uncompressed source. This change ensures we always seek the uncompressed stream to the correct offset when starting an upload call. The old code could fail to resume compressed uploads under some conditions. The `progressiveCompressedUploadShouldWork` test purported to exercise this situation. The test, however, contained the same logic error as the code under test. Closes #15669. PiperOrigin-RevId: 455083727 Change-Id: Ie22dacf31f15644c7a83f49776e7a633d8bb4bca * Updated chunker.java file. * Update src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java Co-authored-by: Benjamin Peterson <benjamin@locrian.net> * Update src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java Co-authored-by: Benjamin Peterson <benjamin@locrian.net> * Update src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java Co-authored-by: Benjamin Peterson <benjamin@locrian.net> Co-authored-by: Benjamin Peterson <benjamin@engflow.com> Co-authored-by: Benjamin Peterson <benjamin@locrian.net>
jgao54
reviewed
Aug 25, 2022
| public void seek(long toOffset) throws IOException { | ||
| if (toOffset < offset) { | ||
| if (initialized && toOffset >= offset && !compressed) { | ||
| ByteStreams.skipFully(data, toOffset - offset); |
There was a problem hiding this comment.
Was just reviewing the release notes today for bazel 5.3.0 and came across this.
it looks like with this change, offset no longer updated here when skipFully is called. Just want to sanity check that this is the intentional behavior? (I am not super familiar with bazel internals and how seek is called, but just worried this would result in extra bytes discarded if offset is not updated)
Collaborator
Author
There was a problem hiding this comment.
Thanks; this is a bug. It think it's unlikely to be triggered in practice, since seeking an initialized chunker forward is rare.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
WriteRequest.write_offsetfield has bizarre semantics during compressed uploads as documented in the remote API protos: https://github.com/bazelbuild/remote-apis/blob/3b4b6402103539d66fcdd1a4d945f660742665ca/build/bazel/remote/execution/v2/remote_execution.proto#L241-L248 In particular, the write offset of the firstWriteRequestrefers to the offset in the uncompressed source.This change ensures we always seek the uncompressed stream to the correct offset when starting an upload call. The old code could fail to resume compressed uploads under some conditions. The
progressiveCompressedUploadShouldWorktest purported to exercise this situation. The test, however, contained the same logic error as the code under test.