Skip to content

HBASE-28050 RSProcedureDispatcher to fail-fast for krb auth failures #5391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
virajjasani merged 10 commits into from
Sep 29, 2023
Merged

HBASE-28050 RSProcedureDispatcher to fail-fast for krb auth failures #5391

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3a5bc01
HBASE-28050 RSProcedureDispatcher to fail-fast for SaslException
virajjasani Sep 11, 2023
a61c49f
addendum
virajjasani Sep 12, 2023
4a79223
addendum
virajjasani Sep 12, 2023
8549505
addendum
virajjasani Sep 13, 2023
a8ba2ee
addendum
virajjasani Sep 14, 2023
420fcb6
addendum
virajjasani Sep 14, 2023
8b59b13
addendum
virajjasani Sep 14, 2023
6fbfe4d
addendum
virajjasani Sep 15, 2023
fbea61e
addendum
virajjasani Sep 15, 2023
a8626fa
addendum
virajjasani Sep 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions ...-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.List;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import javax.security.sasl.SaslException;
import org.apache.hadoop.hbase.CallQueueTooBigException;
import org.apache.hadoop.hbase.DoNotRetryIOException;
import org.apache.hadoop.hbase.ServerName;
Expand Down Expand Up @@ -306,6 +307,10 @@ private boolean scheduleForRetry(IOException e) {
serverName, e.toString(), numberOfAttemptsSoFar);
return false;
}
if (isSaslError(e)) {
Copy link
Contributor

@Apache9 Apache9 Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@virajjasani virajjasani Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good @Apache9, addressed in the latest revision

LOG.warn("{} is not reachable; give up", serverName, e);
return false;
}
if (e instanceof RegionServerAbortedException || e instanceof RegionServerStoppedException) {
// A better way is to return true here to let the upper layer quit, and then schedule a
// background task to check whether the region server is dead. And if it is dead, call
Expand All @@ -330,6 +335,44 @@ private boolean scheduleForRetry(IOException e) {
return true;
}

private boolean isSaslError(IOException e) {
if (e instanceof SaslException) {
return true;
}
// check 4 level of cause
Copy link
Contributor

@Apache9 Apache9 Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a for loop here? And why only test 4 levels?

Copy link
Contributor Author

@virajjasani virajjasani Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

@Apache9 Apache9 Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@virajjasani virajjasani Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }

Copy link
Contributor

@Apache9 Apache9 Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean after unwraping, you still need to go back to the get cause loop, not only test one time...

Copy link
Contributor Author

@virajjasani virajjasani Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Throwable cause = e.getCause();
if (cause == null) {
return false;
}
if (isSaslError(cause)) {
return true;
}
cause = cause.getCause();
if (cause == null) {
return false;
}
if (isSaslError(cause)) {
return true;
}
cause = cause.getCause();
if (cause == null) {
return false;
}
if (isSaslError(cause)) {
return true;
}
cause = cause.getCause();
if (cause == null) {
return false;
}
return isSaslError(cause);
}

private boolean isSaslError(Throwable cause) {
Copy link
Contributor

@Apache9 Apache9 Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return cause instanceof IOException && unwrapException(
(IOException) cause) instanceof SaslException;
}

private long getMaxWaitTime() {
if (this.maxWaitTime < 0) {
// This is the max attempts, not retries, so it should be at least 1.
Expand Down