Uh oh!
There was an error while loading. Please reload this page.
HBASE-28128 Reject requests at RPC layer when RegionServer is aborting - #5447
Merged
Conversation
Apache-HBase
commented
Oct 2, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 2, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 2, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 2, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 3, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 3, 2023
💔 -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) { |
Contributor
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); |
Contributor
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 |
Contributor
There was a problem hiding this comment.
What if the region has been reassigned to other region servers already?
| return Optional.of(this); | ||
| } | ||
| @Override |
Contributor
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.
Apache-HBase
commented
Oct 3, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 3, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 3, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 3, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 3, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 3, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 3, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 4, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 4, 2023
🎊 +1 overall
This message was automatically generated. |
ndimiduk
approved these changes
Oct 4, 2023
ndimiduk
left a comment
Member
There was a problem hiding this comment.
The product change looks good to me -- I'm surprised that it's not already there. We're usually pretty good about checking for abort/stopped status before processing, but maybe that's at a layer down.
| } | ||
| } | ||
| private void sendErrorResponseForCall(int id, long totalRequestSize, Span span, String msg, |
| } | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to load data", e); | ||
| synchronized (THROWN_EXCEPTION) { |
Member
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()) { |
Member
There was a problem hiding this comment.
rpcServer.server can be null? I guess it doesn't hurt to check.
ContributorAuthor
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(); |
Member
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.
ContributorAuthor
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!
bbeaudreault
commented
Oct 4, 2023
ContributorAuthor
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) |
bbeaudreault
commented
Oct 4, 2023
ContributorAuthor
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>
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
Sign up for freeto 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.
No description provided.