-
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-22547 Align the config keys and add document for offheap read in HBase Book. #301
Conversation
🎊 +1 overall
This message was automatically generated. |
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.
Few comments added
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -114,7 +114,7 @@ | |||
public static final int DEFAULT_BUFFER_SIZE = 65 * 1024; | |||
|
|||
public static final String MIN_ALLOCATE_SIZE_KEY = | |||
"hbase.ipc.server.reservoir.minimal.allocating.size"; | |||
"hbase.server.reservoir.minimal.allocating.size"; |
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 not part of this patch any more right?
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 think I forget remove the ipc of this config in https://issues.apache.org/jira/browse/HBASE-22598 before. So remove here.
|
||
Next thing to tune is the ByteBuffer pool on the RPC server side. | ||
The buffers from this pool will be used to accumulate the cell bytes and create a result cell block to send back to the client side. | ||
`hbase.ipc.server.reservoir.enabled` can be used to turn this pool ON or OFF. By default this pool is ON and available. HBase will create off heap ByteBuffers |
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.
Oh we had this config also using ipc. .. We could have deprecated this too the same way. Sorry missed in last comment.
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.
Em..seems we can refactor this config as hbase.server.allocator.enabled... the hbase.ipc.server.reservoir.enabled is some confusing ...
and pool them. Please make sure not to turn this OFF if you want end-to-end off-heaping in read path. | ||
If this pool is turned off, the server will create temp buffers on heap to accumulate the cell bytes and make a result cell block. This can impact the GC on a highly read loaded server. | ||
The user can tune this pool with respect to how many buffers are in the pool and what should be the size of each ByteBuffer. | ||
Use the config `hbase.ipc.server.reservoir.initial.buffer.size` to tune each of the buffer sizes. Default is 64 KB for HBase2.x, while it will be changed to 65KB by default for HBase3.x |
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.
In all these new document, can we refer to new config at 1st place. Also say the old one and is deprecated in 3.0
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.
That's OK.
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.
f6ae122
to
5e93acb
Compare
|
||
Next thing to tune is the ByteBuffer pool on the RPC server side: | ||
|
||
NOTE: the config keys which starts with prefix hbase.ipc.server.reservoir are deprecated in HBase3.x. If you are still |
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.
In all these new document, can we refer to new config at 1st place. Also say the old one and is deprecated in 3.0
@anoopsjohn, I've addressed your comment here.
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Ping @anoopsjohn , any other concerns ? I think it's the last subtask of HBASE-21879 until now. Thanks. |
@@ -68,10 +68,18 @@ | |||
// default heap allocator, it will just allocate ByteBuffers from heap but wrapped by an ByteBuff. | |||
public static final ByteBuffAllocator HEAP = ByteBuffAllocator.createOnHeap(); | |||
|
|||
public static final String ALLOCATOR_ENABLED_KEY = "hbase.server.allocator.enabled"; |
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.
We should call it some way which can clear say if this is enabled, it is pooled stuff what we use. Anyways allocator will always be there in RS. It might be HEAP one.
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.
Sorry for the confusing, my fault. let's define the 4 new config keys as:
1. hbase.server.allocator.enabled -> hbase.server.allocator.pool.enabled
2. hbase.server.reservoir.minimal.allocating.size -> hbase.server.allocator.minimal.allocate.size
3. hbase.server.allocator.max.buffer.count
4. hbase.server.allocator.buffer.size
Will change all plances.
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.
💔 -1 overall
This message was automatically generated. |
Updated the patch:
Also checked all the places. @anoopsjohn , PTAL. |
Update the patch:
@anoopsjohn FYI. |
💔 -1 overall
This message was automatically generated. |
No description provided.