HBASE-23113: Improve and add additional Netty configuration for RPC. #1440
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
|
🎊 +1 overall
This message was automatically generated. |
saintstack
reviewed
Apr 6, 2020
| "hbase.ipc.server.bufferhighwatermark"; | ||
|
|
||
| protected static final boolean DEFAULT_SERVER_REUSEADDR = true; | ||
| protected static final int DEFAULT_SERVER_TCP_BACKLOG = 1024; |
There was a problem hiding this comment.
Yeah, this lower end is much lower than the default.
private static final int DEFAULT_LOW_WATER_MARK = 32 * 1024;
private static final int DEFAULT_HIGH_WATER_MARK = 64 * 1024;
public static final WriteBufferWaterMark DEFAULT =
new WriteBufferWaterMark(DEFAULT_LOW_WATER_MARK, DEFAULT_HIGH_WATER_MARK, false);There was a problem hiding this comment.
The TCP_BACKLOG? I think the linux default is often 128 as mentioned in the JIRA? Raising it too high is also often a mistake, so I went with what the original author did. Have another suggestion?
There was a problem hiding this comment.
For the WATER_MARK settings, the previous pr is using:
protected static final int DEFAULT_SERVER_BUFFER_LOW_WATERMARK = 1024;
protected static final int DEFAULT_SERVER_BUFFER_HIGH_WATERMARK = 64 * 1024;
If the number of queued bytes goes above the high mark, then channel.isWritable will return false until the number of outstanding bytes falls below the low water mark.
There was a problem hiding this comment.
The TCP_BACKLOG?
Sorry, no. I had this on the wrong line. I mean DEFAULT_SERVER_BUFFER_LOW_WATERMARK = 1024.
If the number of queued bytes goes above the high mark, then channel.isWritable will return false until the number of outstanding bytes falls below the low water mark.
Right. So I'm asking you (and I guess @SteNicholas as well): why do we need to flush out the buffer down almost completely before we continue receiving data? Is this assuming that HBase clients are generally writing large amounts of data, and this to avoid thrashing back and forth between threads as we fill, flush, fill, flush? A setting this low means we'd have to drain the buffer almost entirely before returning to the user thread.
There was a problem hiding this comment.
@ndimiduk This could improve performance.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
reviewed
Apr 6, 2020
| @@ -45,24 +46,45 @@ | |||
| @InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.CONFIG) | |||
| public class NettyRpcClient extends AbstractRpcClient<NettyRpcConnection> { | |||
|
|
|||
| protected final WriteBufferWaterMark writeBufferWaterMark; | |||
|
|
|||
| protected static final String CLIENT_CONNECT_MAX_RETRIES = "hbase.ipc.client.connect.max.retries"; | |||
There was a problem hiding this comment.
These first three are unused. Any anyway, the parent class manages reading these values from configuration.
There was a problem hiding this comment.
Copy paste error, should just be WATERMARK stuff.
| @@ -258,6 +252,8 @@ private void connect() { | |||
| .option(ChannelOption.SO_KEEPALIVE, rpcClient.tcpKeepAlive) | |||
| .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, rpcClient.connectTO) | |||
| .handler(new BufferCallBeforeInitHandler()).localAddress(rpcClient.localAddr) | |||
| .option(ChannelOption.WRITE_BUFFER_WATER_MARK, | |||
| ((NettyRpcClient) rpcClient).writeBufferWaterMark) | |||
There was a problem hiding this comment.
Good catch, no longer necessary, from an earlier iteration.
| private static final int EVENTLOOP_THREADCOUNT_DEFAULT = 0; | ||
|
|
||
| protected static final String SERVER_TCP_BACKLOG = "hbase.ipc.server.tcpbacklog"; | ||
| protected static final String SERVER_TCP_REUSEADDR = "hbase.ipc.server.tcpreuseaddr"; | ||
| protected static final String SERVER_TCP_NODELAY = "hbase.ipc.server.tcpnodelay"; |
There was a problem hiding this comment.
NODELAY and KEEPALIVE are unused. Also managed by the parent class.
There was a problem hiding this comment.
Same copy paste mistake as above - previous PR had this all in the base class.
| "hbase.ipc.server.bufferhighwatermark"; | ||
|
|
||
| protected static final boolean DEFAULT_SERVER_REUSEADDR = true; | ||
| protected static final int DEFAULT_SERVER_TCP_BACKLOG = 1024; |
There was a problem hiding this comment.
Yeah, this lower end is much lower than the default.
private static final int DEFAULT_LOW_WATER_MARK = 32 * 1024;
private static final int DEFAULT_HIGH_WATER_MARK = 64 * 1024;
public static final WriteBufferWaterMark DEFAULT =
new WriteBufferWaterMark(DEFAULT_LOW_WATER_MARK, DEFAULT_HIGH_WATER_MARK, false);
|
Looks like this one supersedes #679 . |
markrmiller
force-pushed
the
HBASE-23113
branch
from
355d8fe
to
764fc6f
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Thanks, that took the settings as given in the original pr for master it looks. |
|
Let's get the change to expose these configs into branch-2.3. We'll leave the default settings as they are until we have some proof that new defaults are doing some good. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Failures look related. Complaint about netty on construction of RegionServer. |
|
💔 -1 overall
This message was automatically generated. |
@markrmiller you see the comment above? |
|
I'll take a look and update this in a bit. |
|
@markrmiller Any progress here boss? |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Abandoned PR |
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.
No description provided.