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> (cherry picked from commit ce0db915787b58a79109e6faecc6f1daef9f2850) Reviewed-on: http://gerrit.cloudera.org:8080/9610 Reviewed-by: Grant Henke <granthe...@gmail.com> Tested-by: Grant Henke <granthe...@gmail.com> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/542cfd9e Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/542cfd9e Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/542cfd9e Branch: refs/heads/branch-1.7.x Commit: 542cfd9e01f3c606bfc5f9650ceb14c7e5cd4f5b Parents: 62cb06e Author: Todd Lipcon <t...@apache.org> Authored: Mon Mar 5 00:00:55 2018 -0800 Committer: Grant Henke <granthe...@gmail.com> Committed: Tue Mar 13 21:20:13 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/542cfd9e/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/542cfd9e/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();
