HBASE-26807 Unify CallQueueTooBigException special pause with CallDroppedException #4180
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
bbeaudreault
force-pushed
the
HBASE-26807
branch
2 times, most recently
from
8c33f5b to
ccdc709
Compare
bbeaudreault
commented
Mar 7, 2022
| * @param t exception to check | ||
| * @return true if it's a CQTBE, false otherwise | ||
| */ | ||
| public static boolean isCallQueueTooBigException(Throwable t) { |
There was a problem hiding this comment.
these were not used anywhere and i usually find unused code like this confusing when reading through the code base, so decided to delete since this is an IA.Private class.
|
💔 -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. |
2 similar comments
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
HBASE-26807
branch
from
f6f620d to
3e1c589
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
HBASE-26807
branch
from
3e1c589 to
21cbbec
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
HBASE-26807
branch
from
21cbbec to
e8c7cbe
Compare
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
HBASE-26807
branch
from
e8c7cbe to
e20d78b
Compare
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
HBASE-26807
branch
from
e20d78b to
cb6d865
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
requested changes
Mar 24, 2022
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdminBuilder.java
Show resolved
Hide resolved
| * Base class for exceptions thrown when the hbase server is overloaded. | ||
| */ | ||
| @InterfaceAudience.Public | ||
| public class ServerOverloadedException extends HBaseIOException { |
There was a problem hiding this comment.
A note for other reviewers: I think that this inheritance hierarchy should be fine from a backward compatibility perspective, because HBaseIOException extends from IOException.
| @@ -31,6 +31,7 @@ | |||
| import static org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_OPERATION_TIMEOUT; | |||
| import static org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_PAUSE; | |||
| import static org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_PAUSE_FOR_CQTBE; | |||
| import static org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_PAUSE_FOR_SERVER_OVERLOADED; | |||
There was a problem hiding this comment.
This was the source of your discussion around constants? And you still decided to put it in HConstants? :(
There was a problem hiding this comment.
This PR predates my discussion around constants. I added this in HConstants because that's where the existing CQTBE is. It seems like we've come to a conclusion between those who have commented on the discussion thread, so I could update this based on that new convention (still need to do the other followups from that thread)
There was a problem hiding this comment.
My next commit will include moving this to the ConnectionConfiguration in hbase-client, along with making that class LP(CONFIG). If we'd prefer to leave the LP(CONFIG) change for the forthcoming jira from my discussion thread, that's ok too.
| * @param t exception to check | ||
| * @return true if it's a CQTBE, false otherwise | ||
| */ | ||
| public static boolean isCallQueueTooBigException(Throwable t) { |
| @@ -798,9 +798,19 @@ | |||
|
|
|||
| /** | |||
| * Parameter name for client pause value for special case such as call queue too big, etc. | |||
| * @deprecated Since 2.6.0, will be removed in 4.0.0. Please use | |||
There was a problem hiding this comment.
Ah good, you've done exactly this. Except, if you're quick, this will go out in 2.5.0.
There was a problem hiding this comment.
This should be 2.5.0, I think it's getting in
| server is overloaded, CallQueueTooBigException or CallDroppedException. | ||
| Set this property to a higher value than hbase.client.pause if you | ||
| observe frequent CallQueueTooBigException or CallDroppedException from the same | ||
| RegionServer and the call queue there keeps filling up</description> |
There was a problem hiding this comment.
Add a comment here also saying that this configuration was previously called hbase.client.pause.cqtbe. I recommend this because hbase-default.xml is rendered into the online book.
...er/src/test/java/org/apache/hadoop/hbase/client/TestAsyncClientPauseForServerOverloaded.java
Show resolved
Hide resolved
|
Thank you for the review @ndimiduk. My latest commits resolve all of your comments. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
approved these changes
Mar 30, 2022
There was a problem hiding this comment.
I think you're there -- great work @bbeaudreault !
@apurtell do you have any further comments?
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionConfiguration.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionConfiguration.java
Outdated
Show resolved
Hide resolved
| this(className, msg, doNotRetry, false); | ||
| } | ||
|
|
||
| public RemoteWithExtrasException(String className, String msg, final boolean doNotRetry, |
There was a problem hiding this comment.
Oh, it pains me so that this class is IA.Public and it extends a Hadoop class :'(
There was a problem hiding this comment.
Yea I think it'd be cool to remove/deprecate this class if we were to refactor exception handling based on some of the discussion above.
|
Thanks for your review nick! I'll push a commit with the minor tweaks discussed above later today or tomorrow, to give Andrew time to chime in if he has any other feedback to include as well. |
bbeaudreault
force-pushed
the
HBASE-26807
branch
from
e9389d2 to
e8be9ae
Compare
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
commented
Mar 31, 2022
There was a problem hiding this comment.
@ndimiduk The latest push resolve your last comments. I also did one final self review and I think this looks good and in line with our discussion.
I also updated the branch-2 PR based on all of the review feedback here. That PR has 2 commits -- one is the cherry-pick of this PR, and the other implements for blocking client.
One thing to note in the blocking client implementation is my handling of ConnectionConfiguration. I think what I have there makes the most sense in branch-2 since we currently maintain multiple clients there and want to be DRY. But as I said it means there's a slight divergence in implementation between master/branch-2 for ConnectionConfiguration/AsyncConnectionConfiguration. I think that's ok since 1) it wouldn't be the first time, and 2) these are IA.Private classes. But wanted to call it out.
I actually want to clean up the AsyncConnectionConfiguration/ConnectionConfiguration relationship a bit more, but that's out of scope for this jira. I'm going to file a new jira once I loop back to our discussion thread on the dev list to finalize the topic, which is still on my list.
Thanks again for the reviews!
| @@ -98,6 +111,11 @@ private IOException instantiateException(Class<? extends IOException> cls) throw | |||
| cn.setAccessible(true); | |||
| IOException ex = cn.newInstance(this.getMessage()); | |||
| ex.initCause(this); | |||
|
|
|||
| if (ex instanceof HBaseServerException) { | |||
There was a problem hiding this comment.
This should be safe for existing users of hbase.client.pause.cqtbe as long as they upgrade the server first, then the client. If they don't, the boolean value passed in above will be false and we might erroneously downgrade a CQTBE to !isServerOverloaded.
I'm going to look into tightening this up since I think we should honor the fact that the CQTBE constructor currently always passes true for isServerOverloaded.
|
@ndimiduk I pushed a fix for the one concern I had above. Let me know what you think -- this could be one of those cases where I'm over thinking it and actually it would be cleaner to just assume our stated upgrade path of server first, then client. I'll revert or port to my branch-2 PR based on your feedback. Other than this one thing, I think we're good to go! |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
force-pushed
the
HBASE-26807
branch
from
f1abfe5 to
1d360b1
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. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Sorry for the delay @bbeaudreault ; I'm traveling and my schedule has been chaotic. I'll try to get this committed today. |
|
Do you mind putting up PRs for the branch-2 and branch-2.5 backports? Thanks. |
|
No worries, and thanks! I have PR up for branch-2: #4273. I just updated it with the last change from here. Let me check if it applies cleanly to branch-2.5 |
|
We should let the commit hooks run on that PR before merging, it's been a bit |
|
I like to post PRs like "Backport "HBASE-ABC Blah blah" to branch-x.y" and let the pre-commit job run. It also gives folks a chance to comment on a backport and increases transparency in the project operation. Not everyone does this, but I find the pre-commit checks useful for my own confidence and the transparency is an added bonus. In practice, I rarely get comments on a backport PR, unless it's a non-trivial backport and I've solicited additional review. |
|
Oh, one other thing. We don't have a policy requiring that a committer get +1's on backports, only for the initial change. |
|
Sounds good, thanks for the process clarification! |
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.
Creates a ServerOverloadedException as a base class for both exceptions. Creates a new configuration
hbase.client.pause.server.overloadedand deprecateshbase.client.pause.cqtbe. Updates all references to "cqtbe" in code to "serverOverloaded", and changes instanceof checks to check for new base class ServerOverloadedException.This patch mostly applies cleanly to branch-2, but I'm going to have to also re-implement it for the blocking client there. I will submit a PR for that once I get some initial agreement on the approach here.