-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
HADOOP-18592 Sasl connection failure should log remote address #5294
Conversation
💔 -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.
+1
Nice improvement! I'll plan on committing this tomorrow.
CI failures are unrelated.
@@ -736,19 +733,17 @@ public Object run() throws IOException, InterruptedException { | |||
+ UserGroupInformation.getLoginUser().getUserName() + " to " | |||
+ remoteId; | |||
LOG.warn(msg, ex); | |||
throw (IOException) new IOException(msg).initCause(ex); | |||
throw new IOException(msg, ex); |
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 about using NetUtils.wrapException which is how we should be reporting this stuff -it includes port info too where specified, and includes links to the wiki pages to help people debug it
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.
Destination address host:port would be included with this exception. However, yes NetUtils.wrapException
is excellent utility and worth using here as it also covers wider variety of exceptions and updates the error msg accordingly.
9c1a1a1
to
9adc931
Compare
💔 -1 overall
This message was automatically generated. |
💔 -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.
i like the move to wrap exception, now I'm just proposing expanding it to get the passed in ex so preserve the type of the exception as it goes down. this keeps the information and removes one level of exception wrapping...
remoteId.getAddress().getPort(), | ||
NetUtils.getHostname(), | ||
0, | ||
new IOException(msg, ex)); |
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.
+1 for the change.
ex is only ever an IOException; we should preserve it so that the type doesn't get lost in the chain.
- change the signature of the method to be
IOException ex
and then pass it into wrap exception
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.
Oh yes, it is already of type IOException, should have seen this earlier and would not have got lost in the stacktrace..
Let me make this change, thanks.
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.
preserve the type of the exception as it goes down. this keeps the information and removes one level of exception wrapping
Agree, updated the PR.
💔 -1 overall
This message was automatically generated. |
@steveloughran, updated the PR based on your latest review. |
@cnauroth @steveloughran Does this look good to go? Thanks |
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.
+1
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.
+1
Thank you, @virajjasani . I can do the commit later this week.
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.
LGTM
+1
Thanks for the review @steveloughran |
Contributed by Viraj Jasani <vjasani@apache.org> Signed-off-by: Chris Nauroth <cnauroth@apache.org> Signed-off-by: Steve Loughran <stevel@apache.org> Signed-off-by: Mingliang Liu <liuml07@apache.org>
@liuml07 , thanks for committing. I got delayed. @virajjasani , thank you for the patch! |
…che#5294) Contributed by Viraj Jasani <vjasani@apache.org> Signed-off-by: Chris Nauroth <cnauroth@apache.org> Signed-off-by: Steve Loughran <stevel@apache.org> Signed-off-by: Mingliang Liu <liuml07@apache.org>
If Sasl connection fails with some generic error, we miss logging remote server that the client was trying to connect to.
Sample log:
We should log the remote server address.