feat(gax): add RewindableStreamBuffer for single-chunk rewinds and seeks - #14224
feat(gax): add RewindableStreamBuffer for single-chunk rewinds and seeks#14224whowes wants to merge 5 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces RewindableStreamBuffer, a new stream buffer supporting single-chunk rewind and seeking over an InputStream for resumable uploads, along with its corresponding unit tests. The feedback suggests adding a precondition check in readChunk to ensure the requested targetOffset matches the actual streamPosition to prevent potential data corruption. Additionally, it is recommended to remove the unused chunkSize field and constructor parameter from both the class and its tests.
|
|
||
| if (endOfStream) { | ||
| return ByteString.EMPTY; | ||
| } |
There was a problem hiding this comment.
If targetOffset does not match streamPosition when reading a new chunk from the stream, it means we are attempting to read from an incorrect position in the underlying stream (either because we skipped bytes without seeking, or because we are trying to read an invalid offset). Without this check, the buffer would silently read incorrect data from the stream and associate it with the wrong offset, leading to silent data corruption.
Adding a precondition check ensures that the caller's requested offset matches the actual stream position before reading.
checkArgument(
targetOffset == streamPosition,
"targetOffset (%s) must match streamPosition (%s) when reading from the stream",
targetOffset,
streamPosition);
if (endOfStream) {
return ByteString.EMPTY;
}| private final InputStream source; | ||
| private final int chunkSize; | ||
|
|
||
| private byte @Nullable [] currentChunkData; | ||
| private long currentChunkStartOffset = 0L; | ||
| private long streamPosition = 0L; | ||
| private boolean endOfStream = false; | ||
|
|
||
| public RewindableStreamBuffer(InputStream source, int chunkSize) { | ||
| this.source = checkNotNull(source); | ||
| checkArgument(chunkSize > 0, "chunkSize must be > 0"); | ||
| this.chunkSize = chunkSize; | ||
| } |
There was a problem hiding this comment.
The chunkSize field and constructor parameter are completely unused in this class. To simplify the API and avoid dead code, we should remove them.
private final InputStream source;
private byte @Nullable [] currentChunkData;
private long currentChunkStartOffset = 0L;
private long streamPosition = 0L;
private boolean endOfStream = false;
public RewindableStreamBuffer(InputStream source) {
this.source = checkNotNull(source);
}| import java.nio.charset.StandardCharsets; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class RewindableStreamBufferTest { |
There was a problem hiding this comment.
If the unused chunkSize parameter is removed from the RewindableStreamBuffer constructor, please update all constructor calls in this test file to omit the second argument (e.g., new RewindableStreamBuffer(stream)), and remove the testInvalidChunkSize test case.
e73249a to
5407505
Compare
4b3b68b to
ddfd13c
Compare
3d41927 to
4ba5af0
Compare
ddfd13c to
3c74cf4
Compare
|
❌ The last analysis has failed. |
3c74cf4 to
fec6fbb
Compare
4ba5af0 to
575e38e
Compare
575e38e to
6e8ee1a
Compare
fec6fbb to
42d7132
Compare
6e8ee1a to
be5a011
Compare
42d7132 to
f92d57e
Compare
be5a011 to
849f448
Compare
849f448 to
1fac6c2
Compare
|
|
cf08aec to
87b9732
Compare
1 participant
Work in progress - not yet ready for review