HBASE-28128 Reject requests at RPC layer when RegionServer is aborting #5447
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. |
|
🎊 +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
Oct 3, 2023
| serverWithoutMeta.abort("Abort RS for test"); | ||
|
|
||
| // Wait for our expected exception to be thrown | ||
| synchronized (THROWN_EXCEPTION) { |
There was a problem hiding this comment.
Usually a wait should be in a while loop. Since this is just in a tests, let's just use a UTIL.waitFor to check the exception, and in this way we could have a timeout here.
| } | ||
|
|
||
| private void submitRequestsToRegion(Table table, RegionInfo regionInfo) throws IOException { | ||
| byte[] rowKey = getRowKeyWithin(regionInfo); |
There was a problem hiding this comment.
Better at least add a sleep here? It could be very small but it will help reduce the pressure a lot...
| i++; | ||
| table.put(new Put(rowKey).addColumn(CF, Bytes.toBytes(i), Bytes.toBytes(i))); | ||
| } catch (IOException e) { | ||
| // only catch RegionServerAbortedException once. After that, the next exception thrown |
There was a problem hiding this comment.
What if the region has been reassigned to other region servers already?
| return Optional.of(this); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
OK, we have a sleep here.
But synchronized on a AtomiceReference may trigger some checkstyle warnings, but to have another lock object, or just use sleep and check here, since we are in a test.
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
approved these changes
Oct 4, 2023
| @@ -656,6 +663,15 @@ protected void processRequest(ByteBuff buf) throws IOException, InterruptedExcep | |||
| } | |||
| } | |||
|
|
|||
| private void sendErrorResponseForCall(int id, long totalRequestSize, Span span, String msg, | |||
| } | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to load data", e); | ||
| synchronized (THROWN_EXCEPTION) { |
There was a problem hiding this comment.
Maybe a CountdownLatch is less ham-handed than the synchronized/notify/waitFor dance?
| // fail at the handler layer, but worse might result in CallQueueTooBigException if the | ||
| // queue is full but server is not properly processing requests. Better to throw an aborted | ||
| // exception here so that the client can properly react. | ||
| if (rpcServer.server != null && rpcServer.server.isAborted()) { |
There was a problem hiding this comment.
rpcServer.server can be null? I guess it doesn't hurt to check.
There was a problem hiding this comment.
This comes from tests actually, a few pass in null and were causing NPEs. I figured it was safest to null check it rather than mock all of those cases.
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(TestRegionServerRejectDuringAbort.class); | ||
|
|
||
| private static final HBaseTestingUtil UTIL = new HBaseTestingUtil(); |
There was a problem hiding this comment.
The HBaseTestingUtil has a library of helper methods that you may find useful. For example, createMultiRegionTable and loadRegion.
There was a problem hiding this comment.
Yea, that's useful for setting up a particular state (which is often the case when doing lots of writes in a test). In this case I simply chose put as the api to call to get a CQTBE when i force an abort. I'm not actually caring about or reading the data, I probably could have used get on an empty instead. Thanks for the pointer!
Yea, we check this in pretty much every RSRpcServices endpoint. The problem is that's post-queue, and this adds the check pre-queue. Otherwise if the RS is fully hosed we will end up throwing CallQueueTooBigException (doesnt clear meta) when it really should be ServerAborted (does clear meta) |
|
The test failure is unrelated. @Apache9 let me know if you have any other concerns, otherwise I'll merge tomorrow |
bbeaudreault
added a commit
that referenced
this pull request
Oct 5, 2023
#5447) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Reviewed-by: Duo Zhang <zhangduo@apache.org>
bbeaudreault
added a commit
that referenced
this pull request
Oct 5, 2023
#5447) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Reviewed-by: Duo Zhang <zhangduo@apache.org>
bbeaudreault
added a commit
that referenced
this pull request
Oct 5, 2023
#5447) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Reviewed-by: Duo Zhang <zhangduo@apache.org>
bbeaudreault
added a commit
to HubSpot/hbase
that referenced
this pull request
Oct 5, 2023
…nServer is aborting (apache#5447) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Reviewed-by: Duo Zhang <zhangduo@apache.org>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Nov 2, 2023
apache#5447) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Reviewed-by: Duo Zhang <zhangduo@apache.org>
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.