Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ public void operationComplete(ChannelFuture future) throws Exception {
private void sendRequest0(Call call, HBaseRpcController hrc) throws IOException {
assert eventLoop.inEventLoop();
if (reloginInProgress) {
throw new IOException("Can not send request because relogin is in progress.");
throw new IOException(RpcConnectionConstants.RELOGIN_IS_IN_PROGRESS);
}
hrc.notifyOnCancel(new RpcCallback<Object>() {

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.hadoop.hbase.ipc;

import org.apache.yetus.audience.InterfaceAudience;

/**
* Constants to be used by RPC connection based utilities.
*/
@InterfaceAudience.Private
public final class RpcConnectionConstants {

private RpcConnectionConstants() {
}

public static final String RELOGIN_IS_IN_PROGRESS =
"Can not send request because relogin is in progress.";

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,13 @@
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;
import org.apache.hadoop.hbase.client.AsyncRegionServerAdmin;
import org.apache.hadoop.hbase.client.RegionInfo;
import org.apache.hadoop.hbase.ipc.RpcConnectionConstants;
import org.apache.hadoop.hbase.ipc.ServerNotRunningYetException;
import org.apache.hadoop.hbase.master.MasterServices;
import org.apache.hadoop.hbase.master.ServerListener;
Expand Down Expand Up @@ -287,17 +289,7 @@ private boolean scheduleForRetry(IOException e) {
numberOfAttemptsSoFar);
return false;
}
// 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
// server. Maybe the region server has accepted our request the first time, and then there is
// a network error which prevents we receive the response, and the second time we hit a
// CallQueueTooBigException, obviously it is not safe to quit here, otherwise it may lead to a
// double assign...
if (e instanceof CallQueueTooBigException && numberOfAttemptsSoFar == 0) {
LOG.warn("request to {} failed due to {}, try={}, this usually because"
+ " server is overloaded, give up", serverName, e.toString(), numberOfAttemptsSoFar);
if (numberOfAttemptsSoFar == 0 && unableToConnectToServer(e)) {
return false;
}
// Always retry for other exception types if the region server is not dead yet.
Expand Down Expand Up @@ -330,6 +322,73 @@ private boolean scheduleForRetry(IOException e) {
return true;
}

private boolean unableToConnectToServer(IOException e) {
// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// server. Maybe the region server has accepted our request the first time, and then there is
// a network error which prevents we receive the response, and the second time we hit a
// CallQueueTooBigException, obviously it is not safe to quit here, otherwise it may lead to a
// double assign...
if (e instanceof CallQueueTooBigException) {
LOG.warn("request to {} failed due to {}, try={}, this usually because"
+ " server is overloaded, give up", serverName, e, numberOfAttemptsSoFar);
return true;
}
if (isSaslError(e)) {
LOG.warn("{} is not reachable; give up after first attempt", serverName, e);
return true;
}
return false;
}

private boolean isSaslError(IOException e) {
if (
e instanceof SaslException || (e.getMessage() != null
&& e.getMessage().contains(RpcConnectionConstants.RELOGIN_IS_IN_PROGRESS))
) {
return true;
}
// check 4 level of cause
Copy link
Contributor

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

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

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

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

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

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

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.

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

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