-
Notifications
You must be signed in to change notification settings - Fork 4k
protobuf, api, core, netty: zero copy into protobuf #7330
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
Changes from 1 commit
9936df1
aa93e1d
454e224
3f47d5e
b3c9974
2ede525
3dca312
6249353
29dce67
22ea6c3
53e347c
27801fe
eb71a68
5d3c657
9fd8d3c
e3afe50
e2fdd07
033270b
0622d51
0e8caee
ba4e91b
69618b2
437857d
1363505
c46eb73
7b4e070
772b3ba
b1c99e5
692076c
10c13b8
f13c165
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
… used for okhttp as well.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,13 +31,21 @@ | |
| // TODO(chengyuanzhang): add ExperimentalApi annotation. | ||
| public interface HasByteBuffer { | ||
|
|
||
| /** | ||
| * Indicates whether or not {@link #getByteBuffer} operation is supported. | ||
| */ | ||
| boolean getByteBufferSupported(); | ||
|
|
||
| /** | ||
| * Gets a {@link ByteBuffer} containing some bytes of the content next to be read, or {@code | ||
| * null} if has reached end of the content. The number of bytes contained in the returned buffer | ||
| * is implementation specific. Calling this method does not change the position of the input | ||
| * stream. The returned buffer's content should not be modified, but the position, limit, and | ||
| * mark may be changed. Operations for changing the position, limit, and mark of the returned | ||
| * buffer does not affect the position, limit, and mark of this input stream. | ||
| * buffer does not affect the position, limit, and mark of this input stream. This is an optional | ||
| * method, so callers should first check {@link #getByteBufferSupported}. | ||
| * | ||
| * @throws UnsupportedOperationException if this operation is not supported. | ||
| */ | ||
| @Nullable | ||
| ByteBuffer getByteBuffer(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To make the API cleaner how about having this return
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The semantics of returning |
||
|
|
||
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.
Why not give this the same name as the equivalent added
ReadableBuffermethod (my suggestion would behasByteBuffer())?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.
It doesn't need to match what we use for
ReadableBufferright? We'd probably have a discussion on this API this Thursday and we usually do a vote for the name.hasByteBuffer1 vote now.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.
As well as for consistency, having it line up with
ReadableBufferwould permitReadableBuffers to themselves beInputStreams implementing this interface, which I think might allow for some other streamlining later.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.
Maybe, for that reason.
My original thought was this
HasByteBufferinterface should only have thegetByteBuffermethod. InputStreams not supporting the operation should just not implement this interface. #7330 (comment) suggests combining two InputStream implementations into one by adding thisgetByteBufferSupportedmethod. This look ok, but making the methodhasByteBufferdoes make this API wired. I'd rather change thehasByteBuffermethod onReadableBufferinterface togetByteBufferSupported.