feat: update GrpcBlobReadChannel to allow seek/limit after read #1834
Conversation
This file contains 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
product-auto-label
bot
added
size: l
BenWhitehead
force-pushed
the
grpc/read-channel/seek-after-read
branch
from
c92ac2c
to
3310f2c
Compare
### Refactoring Extract the majority of BlobReadChannelV2 to a new abstract base class BaseStorageReadChannel which effectively adapts a LazyReadChannel to a StorageReadChannel. BaseStorageReadChannel now defines an abstract method newLazyReadChannel which each implementation is responsible for implementing to integrate into the lifecycle.
BenWhitehead
force-pushed
the
grpc/read-channel/seek-after-read
branch
from
3310f2c
to
ce06f6f
Compare
frankyn
reviewed
Jan 11, 2023
| } | ||
| } catch (IOException e) { | ||
| throw e; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
This is broad; is there a specific set of Exception types that are expected?
There was a problem hiding this comment.
For grpc, the scope is more narrow, but for JSON there isn't really a common base since things can be socket issues, ssl issues or any other number of exceptions.
This catch is primarily here to guarantee we're always doing our best to present an actionable exception to whatever is catching.
|
|
||
| protected abstract LazyReadChannel<T> newLazyReadChannel(); | ||
|
|
||
| private void maybeResetChannel(boolean umallocBuffer) throws IOException { |
There was a problem hiding this comment.
what does umallocBuffer mean in this case; is it just to effectively delete the old buffer and start with a new one?
If that's the case, can you name it freeBuffer?
There was a problem hiding this comment.
Yes, I'll rename to freeBuffer
frankyn
approved these changes
Jan 12, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Refactoring
Extract the majority of BlobReadChannelV2 to a new abstract base class BaseStorageReadChannel which effectively adapts a LazyReadChannel to a StorageReadChannel.
BaseStorageReadChannel now defines an abstract method newLazyReadChannel which each implementation is responsible for implementing to integrate into the lifecycle.