Chunker: Always seek on the uncompressed stream. #15720
Closed
Conversation
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
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
Contributor
|
This probably requires the refactor commit. @benjaminp can you help @ckolli5 get the right commits for this cherry-pick? |
benjaminp
reviewed
Jul 1, 2022
| checkState(offset == 0); | ||
| checkState(chunkCache == null); | ||
| try { | ||
| var src = dataSupplier.get(); |
Collaborator
There was a problem hiding this comment.
You're probably going to have to write InputStream src = dataSupplier.get(), since Bazel 5 requires the Java 8 language level.
Author
There was a problem hiding this comment.
Hello @benjaminp, I tried that but still the builds are failing. Here is the PR with the suggested changes. Could you please help us in cherry picking these changes. Thanks!
Merged
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.Closes #15669.
PiperOrigin-RevId: 455083727
Change-Id: Ie22dacf31f15644c7a83f49776e7a633d8bb4bca