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 <[email protected]> Reviewed-by: Alexey Serbin <[email protected]> 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 <[email protected]> Authored: Mon Mar 5 00:00:55 2018 -0800 Committer: Todd Lipcon <[email protected]> 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();
