HBASE-27947 RegionServer OOM when outbound channel backed up #5350
Conversation
This file contains hidden or 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. |
bbeaudreault
force-pushed
the
HBASE-27947
branch
from
dc2bebc to
4072564
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. |
Apache9
reviewed
Aug 13, 2023
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerWrapper.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private boolean handleFatalThreshold(ChannelHandlerContext ctx) { | ||
| int fatalThreshold = rpcServer.getWriteBufferFatalThreshold(); |
Contributor
There was a problem hiding this comment.
We just need this threshold? Let's just pass the threshold in?
Contributor
Author
There was a problem hiding this comment.
Not sure what you mean here. You mean instead of exposing a getter?
I want to use a getter here so that the fatal threshold can be live updated with update_config. If I passed it into the NettyRpcServerResponseEncoder, it would be static for the lifetime of a connection. I could pass in an IntSupplier instead, but is that much better than passing in the NettyRpcServer? I'm happy to do that if preferred
Contributor
There was a problem hiding this comment.
Let's pass an IntSupplier and add comment to say we want to support update_config so we can not pass the threshold in directly.
| return false; | ||
| } | ||
|
|
||
| NettyServerRpcConnection conn = NettyServerRpcConnection.get(channel); |
Contributor
There was a problem hiding this comment.
Just pass the connection in when creating the encoder? Like what we have done in decoder?
Contributor
Author
There was a problem hiding this comment.
The decoder is created after the NettyServerRpcConnection is constructed. The encoder unfortunately is created in NettyRpcServer when the channel is first initialized, before the NettyServerRpcConnection is constructed. Maybe we can refactor a bit?
Contributor
There was a problem hiding this comment.
Let's refactor. Using different pattern for encoder and decoder will make others confusing...
| // when SslHandler is enabled, as it prefers to send a close_notify to the client first. | ||
| channel.config().setOption(ChannelOption.SO_LINGER, 0); | ||
| NettyUnsafeUtils.closeDirect(channel); | ||
| aborted = true; |
Contributor
There was a problem hiding this comment.
After thte above close, channel.isOpen could still returns true? Otherwise we do not need to test aborted in the below isConnectionOpen method?
Contributor
Author
There was a problem hiding this comment.
TBH I'm not 100% sure. I added this just to be sure, but I can try digging deeper in the code to see where in process isConnectionOpen() would turn false.
Contributor
Author
There was a problem hiding this comment.
Ok I traced through the code.
- Since this must be called in the event loop, we don't need to worry about thread safety
- Since we set SO_LINGER to 0, the call to closeDirect should
socket.close() - This will cause
isConnectionOpen()to return false
So we should be ok to remove the aborted boolean. I'm going to run it through my test case and see if I noticed any duplicate logging or anything to indicate otherwise.
Contributor
Author
There was a problem hiding this comment.
Done. Didn't see any issues in tests.
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/ServerRpcConnection.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 15, 2023
hbase-common/src/main/java/org/apache/hadoop/hbase/util/NettyUnsafeUtils.java
Outdated
Show resolved
Hide resolved
hbase-hadoop-compat/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServerWrapper.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcServer.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcServer.java
Outdated
Show resolved
Hide resolved
… the handlers rather than assert eventLoop in connection
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
@Apache9 I just pushed a change which includes a bit of a refactor:
Let me know what you think |
bbeaudreault
commented
Aug 15, 2023
| @InterfaceAudience.Private | ||
| protected NettyRpcServerPreambleHandler createNettyRpcServerPreambleHandler() { | ||
| return new NettyRpcServerPreambleHandler(NettyRpcServer.this); | ||
| protected NettyServerRpcConnection createNettyServerRpcConnection(Channel channel) { |
Contributor
Author
There was a problem hiding this comment.
this change is no longer strictly necessary, but i kept it because i think it's a cleaner way to do what we want to do. in all cases, tests were overriding createNettyRpcServerPreambleHandler() just so they could inject a special NettyServerRpcConnection. So it works better to simply override the creation of the NettyServerRpcConnection directly.
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
I went through and deployed this refactor into my test cluster. Still works as expected. One thing to note is that when we abort the connection, the This gets spammed for all of the requests that were in flight when the connection was closed. I wonder if I should add a |
|
🎊 +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. |
Apache9
approved these changes
Aug 16, 2023
Contributor
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
+1.
Thanks for tuning this!
normanmaurer
suggested changes
Aug 17, 2023
Contributor
normanmaurer
left a comment
normanmaurer
left a comment
There was a problem hiding this comment.
Left 2 comments related to issues that I noticed while reviewing this.
| public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) | ||
| throws Exception { | ||
| if (handleFatalThreshold(ctx)) { | ||
| promise.setFailure(EXCEPTION); |
Contributor
There was a problem hiding this comment.
You must also release the msg via ReferenceCountUtil.release(msg) before return early as otherwise you might leak memory.
Contributor
Author
There was a problem hiding this comment.
Good catch on this, thank you
| public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) | ||
| throws Exception { | ||
| if (handleFatalThreshold(ctx)) { | ||
| promise.setFailure(EXCEPTION); |
Contributor
There was a problem hiding this comment.
Also as a side note reusing a static exception instance is only safe if the instance does not support addSuppressed(...) as otherwise it is possible that you will end up with a memory leak if someone add suppressed exceptions to the static instance, which is possible in this case as you dont know what listeners etc will do.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
added a commit
that referenced
this pull request
Aug 18, 2023
Signed-off-by: Duo Zhang <[email protected]> Reviewed-by: Norman Maurer <[email protected]>
bbeaudreault
added a commit
that referenced
this pull request
Aug 18, 2023
Signed-off-by: Duo Zhang <[email protected]> Reviewed-by: Norman Maurer <[email protected]>
bbeaudreault
added a commit
to HubSpot/hbase
that referenced
this pull request
Aug 21, 2023
…5350) Signed-off-by: Duo Zhang <[email protected]> Reviewed-by: Norman Maurer <[email protected]>
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…5350) Signed-off-by: Duo Zhang <[email protected]> Reviewed-by: Norman Maurer <[email protected]> (cherry picked from commit 29ecfc5) Change-Id: I7ab0cdcbb9c8ee615ad50aacad46145ab50c566f
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
If a client is not able to read response bytes from the channel faster than the server can serve them, netty's pending outbound bytes will build up. If it builds up long enough, it will result in an OOM. We protect against this with the following:
channel.setAutoRead(false). When the high watermark is exceeded, autoRead is disabled. When autoRead is disabled, the server will not continue to read incoming bytes from the client, thus will not enqueue more calls from the client.fatalthreshold, which should be higher than the high watermark. If exceeded, the connection will be forcibly closed so that we can reclaim the memory.Adds new configs:
All 3 default to 0. The setAutoRead management is disabled if both
highandloware 0. Enforcing of the fatal threshold is disabled iffatalis 0. Thus this new behavior is disabled by default.Adds new metrics:
A version of this has been tested internally at my company. We set the fatal limit to 100mb, and were able to avoid any OOMs in our test case.
I still need to add tests to AbstractTestIPC, but wanted to get feedback first.