-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-28647 Support streams in REST Client, RemoteHTable and RemoteAdmin #6010
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 do not fully understand what does the streams mean here...
All requests and responses are fully kept in memory here I think?
* For sending a Protobuf encoded object via Apache HttpClient efficiently without an interim byte | ||
* array. This exposes the underlying Apache HttpClient types, but so do the other client classes. | ||
*/ | ||
@InterfaceAudience.Public |
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.
Does this need to be IA.Public?
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, we expect the user to explicitly create these entities.
i have not updated the tests to use them, but the intentended usage is similar to how
org.apache.hadoop.hbase.rest.client.RemoteHTable.put(Put) is implemented.
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.
At least have a section in the javadoc to show how to use this class?
And since we have not reach an agreement on whether to move RemoteAdmin and RemoteHTable to src/main, I'm not sure whether we should make this class IA.Public...
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 only mentined RemoHTable as example where this is used, but this
is not limited to RemotHTable, it should also be used directly in user code.
I will add a standalone test case that uses this class as an illustration.
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.
At least have a section in the javadoc to show how to use this class?
And since we have not reach an agreement on whether to move RemoteAdmin and RemoteHTable to src/main, I'm not sure whether we should make this class IA.Public...
This is not tied to RemoteAdmin / RemoteHTable.
This is just a more efficient way to use the existing public Client class (or any custom client based on Apache HttpClient)
The RemoteAdmin / RemoteHTable API does not even change, this new class only used internally in the RemoteHTable implementation.
|
||
@Override | ||
public boolean isStreaming() { | ||
// TODO Auto-generated method stub |
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.
Remove this line?
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.
Sure
Currently they are. However, the goal is to avoid having to do that. Protobuf primarily works on streams, so for a large resultset, we may reduce both processing (wall clock) time and memory consumption by not buffering the whole response into memory, but reading directly from the stream, so that
The Cell/Cellset structures are still kept in memory, but we avoid having to explicitly store them twice during processing (once the serialized byte array and once the java POJOs) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Please fix the javac and javadoc warnings if possible? |
I have added org.apache.hadoop.hbase.rest.TestGetAndPutResource.testMultipleCellPutPBEntity() to illustrate how ProtobufEntity can be used to more efficiently execute PUT operations. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fixed the warnings (though most were in code that I haven't touched) |
Can you take a look @Apache9 ? |
Now that you're back from vacation and hopefully settled can you take a look at this again, @Apache9 ? |
path.append('/'); | ||
path.append("dummy_row"); | ||
HttpPut httpPut = new HttpPut(path.toString()); | ||
httpPut.setEntity(new ProtobufHttpEntity(model)); |
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 how a normal client can use ProtobufHttpEntity, @Apache9 .
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
No description provided.