-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-23333 Provide call context around timeouts and other failure scenarios #872
Conversation
Initial commit simply include the output of |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice cleanup. Couple of optional nits.
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/BlockingRpcConnection.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/BlockingRpcConnection.java
Outdated
Show resolved
Hide resolved
@@ -78,8 +78,13 @@ protected Call(int id, final Descriptors.MethodDescriptor md, Message param, | |||
|
|||
@Override | |||
public String toString() { | |||
return "callId: " + this.id + " methodName: " + this.md.getName() + " param {" | |||
+ (this.param != null ? ProtobufUtil.getShortTextFormat(this.param) : "") + "}"; | |||
return new ToStringBuilder(this, ToStringStyle.SHORT_PREFIX_STYLE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice..
a56df5f
to
c7953dd
Compare
I found more-or-less what I was looking for over on HBASE-22316. Somehow the piece of stack trace i found in my logs was omitted. Settling for this for now. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the new error message look like? Will it be too long as we log the whole call?
@@ -80,6 +80,8 @@ public void testRecordStackTrace() throws IOException { | |||
startsWith("org.apache.hadoop.hbase.util.TestFutureUtils.testRecordStackTrace")); | |||
assertTrue(Stream.of(elements) | |||
.anyMatch(element -> element.toString().contains("--------Future.get--------"))); | |||
} catch (Throwable t) { | |||
throw new AssertionError("Caught unexpected Throwable", t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to ensure that the test fails when the exception we expect isn't thrown. It would be simply fail()
except that api doesn't let me retain the context provided by t
.
Maybe this is too verbose?
|
c7953dd
to
44ef309
Compare
Rebased onto master and reduced the verbosity to just call.id and call.methodName. It would be nice to selectively include parameters, such as all non-default values. Could update the log sites to log less when the redundant values are already in the params... What do you folks think? |
🎊 +1 overall
This message was automatically generated. |
Still have objections @Apache9 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patch seems fine to me, fwiw.
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/Call.java
Outdated
Show resolved
Hide resolved
44ef309
to
d2ffc92
Compare
🎊 +1 overall
This message was automatically generated. |
No description provided.