-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: update GrpcBlobReadChannel to allow seek/limit after read #1834
Conversation
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.
3310f2c
to
ce06f6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that the PR is mainly a refactor while enabling seek after read in gRPC; just have 2 follow-up questions otherwise LGTM.
} | ||
} catch (IOException e) { | ||
throw e; | ||
} catch (Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is broad; is there a specific set of Exception types that are expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll rename to freeBuffer
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.