HBASE-21862 IPCUtil.wrapException should keep the original exception types for all the connection exceptions

Signed-off-by: Michael Stack <stack@apache.org>

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<Class<? extends Throwable>> 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
+   * For test only. Usually you should use the {@link #isConnectionException(Throwable)} method
-   * contact/communicate with the server.
+   * below.
-   *
+   */
+  @VisibleForTesting
+  public static Set<Class<? extends Throwable>> 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 (Class<? extends Throwable> clazz : CONNECTION_EXCEPTION_TYPES) {
-    // For example, in SocketOutputStream a plain IOException is thrown
+      if (clazz.isAssignableFrom(e.getClass())) {
-    // at times when the channel is closed.
+        return true;
-    return (e instanceof SocketTimeoutException || e instanceof ConnectException
+      }
-      || e instanceof ClosedChannelException || e instanceof SyncFailedException
+    }
-      || e instanceof EOFException || e instanceof TimeoutException
+    return false;
-      || e instanceof CallTimeoutException || e instanceof ConnectionClosingException
-      || e instanceof FailedServerException || e instanceof ConnectionClosedException);
   }
 
   /**

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.exceptions.TimeoutIOException;
-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.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
+   * the exception is thrown and some extra diagnostics information.
-   * ConnectException or SocketTimeoutException, return a new one of the same type; Otherwise return
+   * <p/>
-   * an IOException.
+   * 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) {
+  static IOException wrapException(InetSocketAddress addr, Throwable error) {
-    if (exception instanceof ConnectException) {
+    if (error instanceof ConnectException) {
       // connection refused; include the host:port in the error
-      return (ConnectException) new ConnectException(
+      return (IOException) new ConnectException(
-          "Call to " + addr + " failed on connection exception: " + exception).initCause(exception);
+        "Call to " + addr + " failed on connection exception: " + error).initCause(error);
-    } else if (exception instanceof SocketTimeoutException) {
+    } else if (error instanceof SocketTimeoutException) {
-      return (SocketTimeoutException) new SocketTimeoutException(
+      return (IOException) new SocketTimeoutException(
-          "Call to " + addr + " failed because " + exception).initCause(exception);
+        "Call to " + addr + " failed because " + error).initCause(error);
-    } else if (exception instanceof ConnectionClosingException) {
+    } else if (error instanceof ConnectionClosingException) {
-      return (ConnectionClosingException) new ConnectionClosingException(
+      return (IOException) new ConnectionClosingException(
-          "Call to " + addr + " failed on local exception: " + exception).initCause(exception);
+        "Call to " + addr + " failed on local exception: " + error).initCause(error);
-    } else if (exception instanceof ServerTooBusyException) {
+    } else if (error instanceof ServerTooBusyException) {
       // we already have address in the exception message
-      return (IOException) exception;
+      return (IOException) error;
-    } else if (exception instanceof DoNotRetryIOException) {
+    } 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);
+        "Call to " + addr + " failed on local exception: " + error).initCause(error);
-    } else if (exception instanceof ConnectionClosedException) {
+    } else if (error instanceof ConnectionClosedException) {
-      return (ConnectionClosedException) exception;
+      return (IOException) new ConnectionClosedException(
-    } else if (exception instanceof StoppedRpcClientException) {
+        "Call to " + addr + " failed on local exception: " + error).initCause(error);
-      return (StoppedRpcClientException) exception;
+    } 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(
+      // try our best to keep the original exception type
-          "Call to " + addr + " failed on local exception: " + exception).initCause(exception);
+      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);
     }
   }
 

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.hamcrest.CoreMatchers.instanceOf;
-import static org.junit.Assert.assertTrue;
+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<? extends Throwable> clazz) throws InstantiationException,
+      IllegalAccessException, InvocationTargetException, NoSuchMethodException {
+    try {
+      Constructor<? extends Throwable> c = clazz.getDeclaredConstructor();
+      c.setAccessible(true);
+      return c.newInstance();
+    } catch (NoSuchMethodException e) {
+      // fall through
+    }
+
+    try {
+      Constructor<? extends Throwable> c = clazz.getDeclaredConstructor(String.class);
+      c.setAccessible(true);
+      return c.newInstance("error");
+    } catch (NoSuchMethodException e) {
+      // fall through
+    }
+
+    try {
+      Constructor<? extends Throwable> c = clazz.getDeclaredConstructor(Throwable.class);
+      c.setAccessible(true);
+      return c.newInstance(new Exception("error"));
+    } catch (NoSuchMethodException e) {
+      // fall through
+    }
+
+    try {
+      Constructor<? extends Throwable> c =
+        clazz.getDeclaredConstructor(String.class, Throwable.class);
+      c.setAccessible(true);
+      return c.newInstance("error", new Exception("error"));
+    } catch (NoSuchMethodException e) {
+      // fall through
+    }
+
+    Constructor<? extends Throwable> 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 {
+  public void testWrapConnectionException() throws Exception {
-    final InetSocketAddress address = InetSocketAddress.createUnresolved("localhost", 0);
+    List<Throwable> exceptions = new ArrayList<>();
-    assertTrue(wrapException(address, new ConnectException()) instanceof ConnectException);
+    for (Class<? extends Throwable> clazz : ClientExceptionsUtil.getConnectionExceptionTypes()) {
-    assertTrue(
+      exceptions.add(create(clazz));
-      wrapException(address, new SocketTimeoutException()) instanceof SocketTimeoutException);
+    }
-    assertTrue(wrapException(address, new ConnectionClosingException(
+    InetSocketAddress addr = InetSocketAddress.createUnresolved("127.0.0.1", 12345);
-        "Test AbstractRpcClient#wrapException")) instanceof ConnectionClosingException);
+    for (Throwable exception : exceptions) {
-    assertTrue(
+      if (exception instanceof TimeoutException) {
-      wrapException(address, new CallTimeoutException("Test AbstractRpcClient#wrapException"))
+        assertThat(IPCUtil.wrapException(addr, exception), instanceOf(TimeoutIOException.class));
-          .getCause() instanceof CallTimeoutException);
+      } else {
+        assertThat(IPCUtil.wrapException(addr, exception), instanceOf(exception.getClass()));
+      }
+    }
   }
 }