You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hbase.apache.org by zh...@apache.org on 2019/02/10 01:44:14 UTC
[hbase] branch master updated: HBASE-21862 IPCUtil.wrapException
should keep the original exception types for all the connection exceptions
This is an automated email from the ASF dual-hosted git repository.
zhangduo pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hbase.git
The following commit(s) were added to refs/heads/master by this push:
new 28bc9a5 HBASE-21862 IPCUtil.wrapException should keep the original exception types for all the connection exceptions
28bc9a5 is described below
commit 28bc9a51e1d323cc9817f196ae23522e36539b51
Author: zhangduo <zh...@apache.org>
AuthorDate: Sun Feb 10 08:15:11 2019 +0800
HBASE-21862 IPCUtil.wrapException should keep the original exception types for all the connection exceptions
Signed-off-by: Michael Stack <st...@apache.org>
---
.../hbase/exceptions/ClientExceptionsUtil.java | 43 ++++++--
.../java/org/apache/hadoop/hbase/ipc/IPCUtil.java | 113 ++++++++++++++-------
.../org/apache/hadoop/hbase/ipc/TestIPCUtil.java | 84 ++++++++++++---
3 files changed, 178 insertions(+), 62 deletions(-)
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 5402a0e..6b1e251 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<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
- * contact/communicate with the server.
- *
+ * For test only. Usually you should use the {@link #isConnectionException(Throwable)} method
+ * 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 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<? extends Throwable> 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 5e38732..c6bbd0b 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.
+ * <p/>
+ * 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 4f6d6aa..ce0d398 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<? 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 {
- 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<Throwable> exceptions = new ArrayList<>();
+ for (Class<? extends Throwable> 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()));
+ }
+ }
}
}