KUDU-2230: java: sync client exception stack traces should make sense

Previously the exceptions thrown by the synchronous client always had
a stack trace somewhere deep inside our async callback chain. So, when
the user eventually caught this exception, it would be very hard to even
know what part of their code failed, since their own code would not even
show up in the call chain.

This patch simply replaces the exception's stack trace with the stack at
the "join" point where the exception is transformed back to a
KuduException. An exception which stores the original stack trace is
attached as a "suppressed" exception.

Change-Id: I874d47b5239bcc8493c15ef763ecfe0fd878d795
Reviewed-on: http://gerrit.cloudera.org:8080/9489
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <danburk...@apache.org>
Reviewed-by: Alexey Serbin <aser...@cloudera.com>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/ce0db915
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/ce0db915
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/ce0db915

Branch: refs/heads/master
Commit: ce0db915787b58a79109e6faecc6f1daef9f2850
Parents: d856107
Author: Todd Lipcon <t...@apache.org>
Authored: Mon Mar 5 00:00:55 2018 -0800
Committer: Todd Lipcon <t...@apache.org>
Committed: Tue Mar 13 18:32:11 2018 +0000

----------------------------------------------------------------------
 .../org/apache/kudu/client/KuduException.java   | 31 ++++++++++++++++----
 .../kudu/client/TestConnectToCluster.java       |  5 +++-
 2 files changed, 30 insertions(+), 6 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/ce0db915/java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
----------------------------------------------------------------------
diff --git 
a/java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java 
b/java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
index 3aa566a..d6dacf2 100644
--- a/java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
+++ b/java/kudu-client/src/main/java/org/apache/kudu/client/KuduException.java
@@ -27,7 +27,6 @@
 package org.apache.kudu.client;
 
 import java.io.IOException;
-import java.util.Arrays;
 
 import com.stumbleupon.async.DeferredGroupException;
 import com.stumbleupon.async.TimeoutException;
@@ -45,7 +44,6 @@ import org.apache.yetus.audience.InterfaceStability;
 @InterfaceStability.Evolving
 @SuppressWarnings("serial")
 public abstract class KuduException extends IOException {
-
   private final Status status;
 
   /**
@@ -77,6 +75,24 @@ public abstract class KuduException extends IOException {
   }
 
   /**
+   * When exceptions are thrown by the asynchronous Kudu client, the stack 
trace is
+   * typically deep within the internals of the Kudu client and/or Netty.
+   * Thus, when the synchronous Kudu client wraps and throws the exception,
+   * we suppress that stack trace and replace it with the stack trace of the 
user's
+   * calling thread. The original stack trace is added to the {@link 
KuduException}
+   * as a suppressed exception ({@see Throwable#addSuppressed(Throwable)}) of
+   * this
+   */
+  @InterfaceAudience.Public
+  @InterfaceStability.Evolving
+  public static class OriginalException extends Throwable {
+    private OriginalException(Throwable e) {
+      super("Original asynchronous stack trace");
+      setStackTrace(e.getStackTrace());
+    }
+  }
+
+  /**
    * Inspects the given exception and transforms it into a KuduException.
    * @param e generic exception we want to transform
    * @return a KuduException that's easier to handle
@@ -85,9 +101,14 @@ public abstract class KuduException extends IOException {
     // The message may be null.
     String message =  e.getMessage() == null ? "" : e.getMessage();
     if (e instanceof KuduException) {
-      // TODO(KUDU-2330) this can lead to methods of the synchronous client
-      // throwing exceptions without the stack trace indicating where
-      // the method was called!
+      // The exception thrown inside the async code has a stack trace
+      // that doesn't correspond to where the user actually called
+      // some synchronous method. This can be very confusing for
+      // users, so we'll reset the stack trace back the call frame
+      // where we are transforming it.
+      e.addSuppressed(new OriginalException(e));
+      StackTraceElement[] stack = new Exception().getStackTrace();
+      e.setStackTrace(stack);
       return (KuduException) e;
     } else if (e instanceof DeferredGroupException) {
       // The cause of a DeferredGroupException is the first exception it sees, 
we're just going to

http://git-wip-us.apache.org/repos/asf/kudu/blob/ce0db915/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
----------------------------------------------------------------------
diff --git 
a/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
 
b/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
index d467c8c..907e987 100644
--- 
a/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
+++ 
b/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
@@ -24,15 +24,16 @@ import static org.junit.Assert.fail;
 
 import java.util.List;
 
+import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.net.HostAndPort;
 import com.stumbleupon.async.Callback;
 
 import org.junit.Assert;
 import org.junit.Test;
-
 import org.apache.kudu.consensus.Metadata;
 import org.apache.kudu.master.Master.ConnectToMasterResponsePB;
+import org.hamcrest.CoreMatchers;
 
 public class TestConnectToCluster {
 
@@ -97,6 +98,8 @@ public class TestConnectToCluster {
                   ".*Client configured with 1 master\\(s\\) " +
                   "\\(.+?\\) but cluster indicates it expects 3 master\\(s\\) 
" +
                   "\\(.+?,.+?,.+?\\).*"));
+          Assert.assertThat(Joiner.on("\n").join(e.getStackTrace()),
+              CoreMatchers.containsString("testConnectToOneOfManyMasters"));
         } finally {
           if (c != null) {
             c.close();

Reply via email to