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 1 commit 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
5407505 to
b706b52
Compare
b706b52 to
3d41927
Compare
|
|
1 participant
Work in progress - not yet ready for review