Skip to content
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

STORM-3749 improve logging for server error #3384

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

agresch
Copy link
Contributor

@agresch agresch commented Mar 4, 2021

What is the purpose of the change

Improve the error message when a server error occurs/

How was the change tested

built storm-client

Copy link
Contributor

@kishorvpatil kishorvpatil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
}

@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
try {
LOG.error("server errors in handling the request", cause);
LOG.error("server errors in handling the request to " + ctx.channel().remoteAddress().toString(), cause);
Copy link
Contributor

@Ethanlm Ethanlm Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change "to" to "from" since this is an ChannelInboundHandlerAdapter?

nit: Use "{}" instead of "+". i.e.

LOG.error("server errors in handling the request from {}", ctx.channel().remoteAddress().toString(), cause);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I found (earlier) that passing parameter in the LOG.error does not handle the exception properly (i.e. stack trace is not printed). This maybe because we use an older version of SL4J. So the error above with {} will print only the message text but not stack. But if there is only one argument then the stack trace is printed. Here is a link that refers to sl4j version where this was fixed http://www.slf4j.org/faq.html#paramException

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore previous comment. I just tested this in a storm test and parameters work in LOG.error correctly as expected.

@agresch agresch merged commit 0859cd1 into apache:master Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants