diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/exceptions/ClientExceptionsUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/exceptions/ClientExceptionsUtil.java index 5402a0e7db5..6b1e251953b 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/exceptions/ClientExceptionsUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/exceptions/ClientExceptionsUtil.java @@ -26,6 +26,7 @@ import java.lang.reflect.UndeclaredThrowableException; import java.net.ConnectException; import java.net.SocketTimeoutException; import java.nio.channels.ClosedChannelException; +import java.util.Set; import java.util.concurrent.TimeoutException; import org.apache.hadoop.hbase.CallDroppedException; @@ -37,6 +38,10 @@ import org.apache.hadoop.hbase.RegionTooBusyException; import org.apache.hadoop.hbase.RetryImmediatelyException; import org.apache.yetus.audience.InterfaceAudience; import org.apache.yetus.audience.InterfaceStability; + +import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting; +import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableSet; + import org.apache.hadoop.hbase.ipc.CallTimeoutException; import org.apache.hadoop.hbase.ipc.FailedServerException; import org.apache.hadoop.hbase.quotas.RpcThrottlingException; @@ -131,10 +136,28 @@ public final class ClientExceptionsUtil { return (t instanceof CallDroppedException); } + // This list covers most connectivity exceptions but not all. + // For example, in SocketOutputStream a plain IOException is thrown at times when the channel is + // closed. + private static final ImmutableSet> CONNECTION_EXCEPTION_TYPES = + ImmutableSet.of(SocketTimeoutException.class, ConnectException.class, + ClosedChannelException.class, SyncFailedException.class, EOFException.class, + TimeoutException.class, TimeoutIOException.class, CallTimeoutException.class, + ConnectionClosingException.class, FailedServerException.class, + ConnectionClosedException.class); + /** - * Check if the exception is something that indicates that we cannot - * contact/communicate with the server. - * + * For test only. Usually you should use the {@link #isConnectionException(Throwable)} method + * below. + */ + @VisibleForTesting + public static Set> getConnectionExceptionTypes() { + return CONNECTION_EXCEPTION_TYPES; + } + + /** + * Check if the exception is something that indicates that we cannot contact/communicate with the + * server. * @param e exception to check * @return true when exception indicates that the client wasn't able to make contact with server */ @@ -142,14 +165,12 @@ public final class ClientExceptionsUtil { if (e == null) { return false; } - // This list covers most connectivity exceptions but not all. - // For example, in SocketOutputStream a plain IOException is thrown - // at times when the channel is closed. - return (e instanceof SocketTimeoutException || e instanceof ConnectException - || e instanceof ClosedChannelException || e instanceof SyncFailedException - || e instanceof EOFException || e instanceof TimeoutException - || e instanceof CallTimeoutException || e instanceof ConnectionClosingException - || e instanceof FailedServerException || e instanceof ConnectionClosedException); + for (Class clazz : CONNECTION_EXCEPTION_TYPES) { + if (clazz.isAssignableFrom(e.getClass())) { + return true; + } + } + return false; } /** diff --git a/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java b/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java index 5e38732b8e1..c6bbd0ba5eb 100644 --- a/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java +++ b/hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/IPCUtil.java @@ -17,28 +17,34 @@ */ package org.apache.hadoop.hbase.ipc; -import org.apache.hadoop.hbase.exceptions.ConnectionClosedException; -import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; -import org.apache.hbase.thirdparty.com.google.protobuf.CodedOutputStream; -import org.apache.hbase.thirdparty.com.google.protobuf.Message; - import java.io.IOException; import java.io.OutputStream; +import java.lang.reflect.InvocationTargetException; import java.net.ConnectException; import java.net.InetSocketAddress; import java.net.SocketTimeoutException; import java.nio.ByteBuffer; - +import java.nio.channels.ClosedChannelException; +import java.util.concurrent.TimeoutException; import org.apache.hadoop.hbase.DoNotRetryIOException; +import org.apache.hadoop.hbase.HBaseIOException; import org.apache.hadoop.hbase.HConstants; -import org.apache.yetus.audience.InterfaceAudience; +import org.apache.hadoop.hbase.exceptions.ClientExceptionsUtil; +import org.apache.hadoop.hbase.exceptions.ConnectionClosedException; import org.apache.hadoop.hbase.exceptions.ConnectionClosingException; -import org.apache.hadoop.hbase.shaded.protobuf.generated.RPCProtos.CellBlockMeta; -import org.apache.hadoop.hbase.shaded.protobuf.generated.RPCProtos.ExceptionResponse; -import org.apache.hadoop.hbase.shaded.protobuf.generated.RPCProtos.RequestHeader; +import org.apache.hadoop.hbase.exceptions.TimeoutIOException; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.EnvironmentEdgeManager; import org.apache.hadoop.ipc.RemoteException; +import org.apache.yetus.audience.InterfaceAudience; + +import org.apache.hbase.thirdparty.com.google.common.base.Preconditions; +import org.apache.hbase.thirdparty.com.google.protobuf.CodedOutputStream; +import org.apache.hbase.thirdparty.com.google.protobuf.Message; + +import org.apache.hadoop.hbase.shaded.protobuf.generated.RPCProtos.CellBlockMeta; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RPCProtos.ExceptionResponse; +import org.apache.hadoop.hbase.shaded.protobuf.generated.RPCProtos.RequestHeader; /** * Utility to help ipc'ing. @@ -93,7 +99,7 @@ class IPCUtil { continue; } totalSize += m.getSerializedSize(); - totalSize += CodedOutputStream.computeRawVarint32Size(m.getSerializedSize()); + totalSize += CodedOutputStream.computeUInt32SizeNoTag(m.getSerializedSize()); } Preconditions.checkArgument(totalSize < Integer.MAX_VALUE); return totalSize; @@ -153,37 +159,74 @@ class IPCUtil { /** * Takes an Exception and the address we were trying to connect to and return an IOException with * the input exception as the cause. The new exception provides the stack trace of the place where - * the exception is thrown and some extra diagnostics information. If the exception is - * ConnectException or SocketTimeoutException, return a new one of the same type; Otherwise return - * an IOException. + * the exception is thrown and some extra diagnostics information. + *

+ * Notice that we will try our best to keep the original exception type when creating a new + * exception, especially for the 'connection' exceptions, as it is used to determine whether this + * is a network issue or the remote side tells us clearly what is wrong, which is very important + * to decide whether to retry. If it is not possible to create a new exception with the same type, + * for example, the {@code error} is not an {@link IOException}, an {@link IOException} will be + * created. * @param addr target address - * @param exception the relevant exception + * @param error the relevant exception * @return an exception to throw + * @see ClientExceptionsUtil#isConnectionException(Throwable) */ - static IOException wrapException(InetSocketAddress addr, Exception exception) { - if (exception instanceof ConnectException) { + static IOException wrapException(InetSocketAddress addr, Throwable error) { + if (error instanceof ConnectException) { // connection refused; include the host:port in the error - return (ConnectException) new ConnectException( - "Call to " + addr + " failed on connection exception: " + exception).initCause(exception); - } else if (exception instanceof SocketTimeoutException) { - return (SocketTimeoutException) new SocketTimeoutException( - "Call to " + addr + " failed because " + exception).initCause(exception); - } else if (exception instanceof ConnectionClosingException) { - return (ConnectionClosingException) new ConnectionClosingException( - "Call to " + addr + " failed on local exception: " + exception).initCause(exception); - } else if (exception instanceof ServerTooBusyException) { + return (IOException) new ConnectException( + "Call to " + addr + " failed on connection exception: " + error).initCause(error); + } else if (error instanceof SocketTimeoutException) { + return (IOException) new SocketTimeoutException( + "Call to " + addr + " failed because " + error).initCause(error); + } else if (error instanceof ConnectionClosingException) { + return (IOException) new ConnectionClosingException( + "Call to " + addr + " failed on local exception: " + error).initCause(error); + } else if (error instanceof ServerTooBusyException) { // we already have address in the exception message - return (IOException) exception; - } else if (exception instanceof DoNotRetryIOException) { + return (IOException) error; + } else if (error instanceof DoNotRetryIOException) { + // try our best to keep the original exception type + try { + return (IOException) error.getClass().asSubclass(DoNotRetryIOException.class) + .getConstructor(String.class) + .newInstance("Call to " + addr + " failed on local exception: " + error).initCause(error); + } catch (InstantiationException | IllegalAccessException | IllegalArgumentException + | InvocationTargetException | NoSuchMethodException | SecurityException e) { + // just ignore, will just new a DoNotRetryIOException instead below + } return (IOException) new DoNotRetryIOException( - "Call to " + addr + " failed on local exception: " + exception).initCause(exception); - } else if (exception instanceof ConnectionClosedException) { - return (ConnectionClosedException) exception; - } else if (exception instanceof StoppedRpcClientException) { - return (StoppedRpcClientException) exception; + "Call to " + addr + " failed on local exception: " + error).initCause(error); + } else if (error instanceof ConnectionClosedException) { + return (IOException) new ConnectionClosedException( + "Call to " + addr + " failed on local exception: " + error).initCause(error); + } else if (error instanceof CallTimeoutException) { + return (IOException) new CallTimeoutException( + "Call to " + addr + " failed on local exception: " + error).initCause(error); + } else if (error instanceof ClosedChannelException) { + // ClosedChannelException does not have a constructor which takes a String but it is a + // connection exception so we keep its original type + return (IOException) error; + } else if (error instanceof TimeoutException) { + // TimeoutException is not an IOException, let's convert it to TimeoutIOException. + return (IOException) new TimeoutIOException( + "Call to " + addr + " failed on local exception: " + error).initCause(error); } else { - return (IOException) new IOException( - "Call to " + addr + " failed on local exception: " + exception).initCause(exception); + // try our best to keep the original exception type + if (error instanceof IOException) { + try { + return (IOException) error.getClass().asSubclass(IOException.class) + .getConstructor(String.class) + .newInstance("Call to " + addr + " failed on local exception: " + error) + .initCause(error); + } catch (InstantiationException | IllegalAccessException | IllegalArgumentException + | InvocationTargetException | NoSuchMethodException | SecurityException e) { + // just ignore, will just new an IOException instead below + } + } + return (IOException) new HBaseIOException( + "Call to " + addr + " failed on local exception: " + error).initCause(error); } } diff --git a/hbase-client/src/test/java/org/apache/hadoop/hbase/ipc/TestIPCUtil.java b/hbase-client/src/test/java/org/apache/hadoop/hbase/ipc/TestIPCUtil.java index 4f6d6aa77f8..ce0d3985723 100644 --- a/hbase-client/src/test/java/org/apache/hadoop/hbase/ipc/TestIPCUtil.java +++ b/hbase-client/src/test/java/org/apache/hadoop/hbase/ipc/TestIPCUtil.java @@ -17,14 +17,18 @@ */ package org.apache.hadoop.hbase.ipc; -import static org.apache.hadoop.hbase.ipc.IPCUtil.wrapException; -import static org.junit.Assert.assertTrue; +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.junit.Assert.assertThat; -import java.net.ConnectException; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; import java.net.InetSocketAddress; -import java.net.SocketTimeoutException; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.TimeoutException; import org.apache.hadoop.hbase.HBaseClassTestRule; -import org.apache.hadoop.hbase.exceptions.ConnectionClosingException; +import org.apache.hadoop.hbase.exceptions.ClientExceptionsUtil; +import org.apache.hadoop.hbase.exceptions.TimeoutIOException; import org.apache.hadoop.hbase.testclassification.ClientTests; import org.apache.hadoop.hbase.testclassification.SmallTests; import org.junit.ClassRule; @@ -36,18 +40,66 @@ public class TestIPCUtil { @ClassRule public static final HBaseClassTestRule CLASS_RULE = - HBaseClassTestRule.forClass(TestIPCUtil.class); + HBaseClassTestRule.forClass(TestIPCUtil.class); + private static Throwable create(Class clazz) throws InstantiationException, + IllegalAccessException, InvocationTargetException, NoSuchMethodException { + try { + Constructor c = clazz.getDeclaredConstructor(); + c.setAccessible(true); + return c.newInstance(); + } catch (NoSuchMethodException e) { + // fall through + } + + try { + Constructor c = clazz.getDeclaredConstructor(String.class); + c.setAccessible(true); + return c.newInstance("error"); + } catch (NoSuchMethodException e) { + // fall through + } + + try { + Constructor c = clazz.getDeclaredConstructor(Throwable.class); + c.setAccessible(true); + return c.newInstance(new Exception("error")); + } catch (NoSuchMethodException e) { + // fall through + } + + try { + Constructor c = + clazz.getDeclaredConstructor(String.class, Throwable.class); + c.setAccessible(true); + return c.newInstance("error", new Exception("error")); + } catch (NoSuchMethodException e) { + // fall through + } + + Constructor c = + clazz.getDeclaredConstructor(Throwable.class, Throwable.class); + c.setAccessible(true); + return c.newInstance(new Exception("error"), "error"); + } + + /** + * See HBASE-21862, it is very important to keep the original exception type for connection + * exceptions. + */ @Test - public void testWrapException() throws Exception { - final InetSocketAddress address = InetSocketAddress.createUnresolved("localhost", 0); - assertTrue(wrapException(address, new ConnectException()) instanceof ConnectException); - assertTrue( - wrapException(address, new SocketTimeoutException()) instanceof SocketTimeoutException); - assertTrue(wrapException(address, new ConnectionClosingException( - "Test AbstractRpcClient#wrapException")) instanceof ConnectionClosingException); - assertTrue( - wrapException(address, new CallTimeoutException("Test AbstractRpcClient#wrapException")) - .getCause() instanceof CallTimeoutException); + public void testWrapConnectionException() throws Exception { + List exceptions = new ArrayList<>(); + for (Class clazz : ClientExceptionsUtil.getConnectionExceptionTypes()) { + exceptions.add(create(clazz)); + } + InetSocketAddress addr = InetSocketAddress.createUnresolved("127.0.0.1", 12345); + for (Throwable exception : exceptions) { + if (exception instanceof TimeoutException) { + assertThat(IPCUtil.wrapException(addr, exception), instanceOf(TimeoutIOException.class)); + } else { + assertThat(IPCUtil.wrapException(addr, exception), instanceOf(exception.getClass())); + } + } } }