Uh oh!
There was an error while loading. Please reload this page.
HBASE-28050 RSProcedureDispatcher to fail-fast for krb auth failures - #5391
Merged
Conversation
Apache-HBase
commented
Sep 12, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 12, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 12, 2023
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Sep 12, 2023
| serverName, e.toString(), numberOfAttemptsSoFar); | ||
| return false; | ||
| } | ||
| if (isSaslError(e)) { |
Contributor
There was a problem hiding this comment.
I think here we need to follow the same pattern with CallQueueTooBigException, only if this is the first try, we can make sure that the task has not been sent yet.
ContributorAuthor
There was a problem hiding this comment.
sounds good @Apache9, addressed in the latest revision
Apache-HBase
commented
Sep 12, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 12, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 12, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 12, 2023
💔 -1 overall
This message was automatically generated. |
| assert eventLoop.inEventLoop(); | ||
| if (reloginInProgress) { | ||
| throw new IOException("Can not send request because relogin is in progress."); | ||
| throw new IOException(HConstants.RELOGIN_IS_IN_PROGRESS); |
Contributor
There was a problem hiding this comment.
This is weird, please don't put these kinds of constants in HConstants. There are too many unrelated concerns there already.
Public static string constant in some other file, even this one, is preferred.
ContributorAuthor
There was a problem hiding this comment.
NettyRpcConnection is package private, hence can't be accessed from hbase-server
ContributorAuthor
There was a problem hiding this comment.
not sure what is the best place to keep this, anywhere else in hbase-common would also work
Contributor
There was a problem hiding this comment.
Please do not put it in HConstants, it is IA.Public.
ContributorAuthor
There was a problem hiding this comment.
i understand but i am not sure what is the best place to keep this in
ContributorAuthor
There was a problem hiding this comment.
ok, this is now taken care of
| serverName, e.toString(), numberOfAttemptsSoFar); | ||
| return false; | ||
| } | ||
| if (isSaslError(e) && numberOfAttemptsSoFar == 0) { |
Contributor
There was a problem hiding this comment.
Does it matter how many attempts we have had so far if now we are getting a SASL error?
ContributorAuthor
There was a problem hiding this comment.
Duo's point is that, if some attempts were already successful, it might have triggered region transition already and we might be in the middle of another sub-procedure when we encounter this.
I also think num of attempts should not matter as we won't be able to make any progress anyways, but then it takes our discussion back to the parent Jira HBASE-28048
Contributor
There was a problem hiding this comment.
Yes, exactly as @virajjasani said, we need to make sure that the remote procedure has not been sent to the rs then we are safe to quit and choose another rs, otherwise the only safe way is to rely on SCP to tell us the rs is dead so we are safe to quit here.
So if we hit another error, like connection timed out the first time, then we are not sure whether we have already send the procedure to the rs, then no matter what the exceptions are in the following retries, we are not safe to quit.
In the real world, if authentication is not configured correctly, then it is likely we will get sasl error at the first try, so the code is enough to cover the problem here.
Contributor
There was a problem hiding this comment.
@virajjasani Better to introduce a method may be called
boolean hasNotReachedRegionServerYet(IOException e)
In this method we could test for both sasl error and call queue too big, and also other exception types in the future. So we do not need to add extra condition every time when we want to add new exception type tests in scheduleForRetry method.
Apache-HBase
commented
Sep 13, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 13, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 13, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 13, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 14, 2023
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Sep 14, 2023
| return true; | ||
| } | ||
| private boolean unableToConnectToServerInFirstAttempt(IOException e) { |
Contributor
There was a problem hiding this comment.
I mean we could extract a method for testing exception only, and then we test numberOfAttemptsSoFar outside the method...
Apache-HBase
commented
Sep 14, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 14, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 14, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 14, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 14, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 14, 2023
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Sep 15, 2023
| // This exception is thrown in the rpc framework, where we can make sure that the call has not | ||
| // been executed yet, so it is safe to mark it as fail. Especially for open a region, we'd | ||
| // better choose another region server. | ||
| // Notice that, it is safe to quit only if this is the first time we send request to region |
Contributor
There was a problem hiding this comment.
Better move this block of comments to the if condition in the caller method? I mean the section start from 'Notice that, it is safe blabla'. The numberOfAttemptsSoFar == 0 test is there.
Uh oh!
There was an error while loading. Please reload this page.
| ) { | ||
| return true; | ||
| } | ||
| // check 4 level of cause |
Contributor
There was a problem hiding this comment.
Use a for loop here? And why only test 4 levels?
ContributorAuthor
There was a problem hiding this comment.
it's based on the examples we have seen so far, e.g.procedure.RSProcedureDispatcher - request to rs1,61020,1692930044498 failed due to java.io.IOException: Call to address=rs1:61020 failed on local exception: java.io.IOException: org.apache.hbase.thirdparty.io.netty.handler.codec.DecoderException: org.apache.hadoop.ipc.RemoteException(javax.security.sasl.SaslException): GSS initiate failed, try=0, retrying...
Contributor
There was a problem hiding this comment.
I think we could just use a loop to get the cause until cause is null, to check all the exceptions on chain. And we also need to handle RemoteException specially, to unwrap it instead of just calling getCause?
ContributorAuthor
There was a problem hiding this comment.
handle RemoteException specially, to unwrap it instead of just calling getCause
yes, that is taken care of:
private boolean isThrowableOfTypeSasl(Throwable cause) {
if (cause instanceof IOException) {
IOException unwrappedException = unwrapException((IOException) cause);
return unwrappedException instanceof SaslException
|| (unwrappedException.getMessage() != null && unwrappedException.getMessage()
.contains(RpcConnectionConstants.RELOGIN_IS_IN_PROGRESS));
}
return false;
}
Contributor
There was a problem hiding this comment.
I mean after unwraping, you still need to go back to the get cause loop, not only test one time...
ContributorAuthor
There was a problem hiding this comment.
Yes, it is in the loop
while (true) {
cause = cause.getCause();
if (cause == null) {
return false;
}
if (isThrowableOfTypeSasl(cause)) {
return true;
}
}
isThrowableOfTypeSasl does the unwrap and checks for type of exception.
Uh oh!
There was an error while loading. Please reload this page.
| return isSaslError(cause); | ||
| } | ||
| private boolean isSaslError(Throwable cause) { |
Contributor
There was a problem hiding this comment.
Please do not use the same method name here, as IOException is also a Throwable, although this is valid in Java, but it will confuse the developers.
Apache-HBase
commented
Sep 15, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 15, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 15, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 15, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 15, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 15, 2023
💔 -1 overall
This message was automatically generated. |
Comment on lines
+354
to
+381
| private boolean isSaslError(IOException e) { | ||
| if ( | ||
| e instanceof SaslException || (e.getMessage() != null | ||
| && e.getMessage().contains(RpcConnectionConstants.RELOGIN_IS_IN_PROGRESS)) | ||
| ) { | ||
| return true; | ||
| } | ||
| Throwable cause = e; | ||
| while (true) { | ||
| cause = cause.getCause(); | ||
| if (cause == null) { | ||
| return false; | ||
| } | ||
| if (isThrowableOfTypeSasl(cause)) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| private boolean isThrowableOfTypeSasl(Throwable cause) { | ||
| if (cause instanceof IOException) { | ||
| IOException unwrappedException = unwrapException((IOException) cause); | ||
| return unwrappedException instanceof SaslException | ||
| || (unwrappedException.getMessage() != null && unwrappedException.getMessage() | ||
| .contains(RpcConnectionConstants.RELOGIN_IS_IN_PROGRESS)); | ||
| } | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Can the unwrapping be simplified, since unwrapException won't do anything unless it's a RemoteException?
Suggested change
| privatebooleanisSaslError(IOExceptione) { | |
| if ( | |
| einstanceofSaslException || (e.getMessage() != null | |
| && e.getMessage().contains(RpcConnectionConstants.RELOGIN_IS_IN_PROGRESS)) | |
| ) { | |
| returntrue; | |
| } | |
| Throwablecause = e; | |
| while (true) { | |
| cause = cause.getCause(); | |
| if (cause == null) { | |
| returnfalse; | |
| } | |
| if (isThrowableOfTypeSasl(cause)) { | |
| returntrue; | |
| } | |
| } | |
| } | |
| privatebooleanisThrowableOfTypeSasl(Throwablecause) { | |
| if (causeinstanceofIOException) { | |
| IOExceptionunwrappedException = unwrapException((IOException) cause); | |
| returnunwrappedExceptioninstanceofSaslException | |
| || (unwrappedException.getMessage() != null && unwrappedException.getMessage() | |
| .contains(RpcConnectionConstants.RELOGIN_IS_IN_PROGRESS)); | |
| } | |
| returnfalse; | |
| } | |
| privatebooleanisSaslError(IOExceptione) { | |
| Throwablecause = e; | |
| while (true) { | |
| if (causeinstanceofIOException) { | |
| IOExceptionunwrappedCause = unwrapException((IOException) cause); | |
| if ( | |
| unwrappedCauseinstanceofSaslException || (unwrappedCause.getMessage() != null | |
| && unwrappedCause.getMessage().contains(RpcConnectionConstants.RELOGIN_IS_IN_PROGRESS)) | |
| ) { | |
| returntrue; | |
| } | |
| } | |
| cause = cause.getCause(); | |
| if (cause == null) { | |
| returnfalse; | |
| } | |
| } | |
| } |
ContributorAuthor
There was a problem hiding this comment.
much better, let me update the PR
Apache-HBase
commented
Sep 16, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 17, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Sep 17, 2023
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Sep 18, 2023
virajjasani
commented
Sep 18, 2023
ContributorAuthor
Thanks for the reviews! Let me merge and backport this today. |
mnpoonia
approved these changes
Sep 18, 2023
d-c-manning
approved these changes
Sep 19, 2023
apurtell
approved these changes
Sep 26, 2023
virajjasani
commented
Sep 29, 2023
ContributorAuthor
Thank you for the reviews @Apache9@mnpoonia@d-c-manning@apurtell !! |
virajjasani added a commit
that referenced
this pull request
Sep 29, 2023
…5391) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Aman Poonia <aman.poonia.29@gmail.com> Signed-off-by: David Manning <david.manning@salesforce.com>
virajjasani added a commit
that referenced
this pull request
Sep 29, 2023
…5391) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Aman Poonia <aman.poonia.29@gmail.com> Signed-off-by: David Manning <david.manning@salesforce.com>
virajjasani added a commit
that referenced
this pull request
Sep 29, 2023
…5391) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Aman Poonia <aman.poonia.29@gmail.com> Signed-off-by: David Manning <david.manning@salesforce.com>
virajjasani added a commit
that referenced
this pull request
Sep 29, 2023
…5391) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Aman Poonia <aman.poonia.29@gmail.com> Signed-off-by: David Manning <david.manning@salesforce.com>
wchevreuil pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Nov 2, 2023
…pache#5391) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Aman Poonia <aman.poonia.29@gmail.com> Signed-off-by: David Manning <david.manning@salesforce.com>
vinayakphegde pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…pache#5391) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Aman Poonia <aman.poonia.29@gmail.com> Signed-off-by: David Manning <david.manning@salesforce.com> (cherry picked from commit 597da71) Change-Id: I7a6dc36ebc525397237fe8f0250a43d7213c9d17
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
6 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.