You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by GitBox <gi...@apache.org> on 2021/03/04 21:41:33 UTC
[GitHub] [storm] agresch opened a new pull request #3384: STORM-3749 improve logging for server error
agresch opened a new pull request #3384:
URL: https://github.com/apache/storm/pull/3384
## What is the purpose of the change
Improve the error message when a server error occurs/
## How was the change tested
built storm-client
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3384: STORM-3749 improve logging for server error
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3384:
URL: https://github.com/apache/storm/pull/3384#discussion_r588370696
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/StormServerHandler.java
##########
@@ -51,14 +48,13 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
server.received(msg, channel.remoteAddress().toString(), channel);
} catch (InterruptedException e) {
LOG.info("failed to enqueue a request message", e);
- failureCount.incrementAndGet();
}
}
@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);
Review comment:
change "to" to "from" since this is an `ChannelInboundHandlerAdapter`?
nit: Use "{}" instead of "+".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3384: STORM-3749 improve logging for server error
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3384:
URL: https://github.com/apache/storm/pull/3384#discussion_r588370696
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/StormServerHandler.java
##########
@@ -51,14 +48,13 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
server.received(msg, channel.remoteAddress().toString(), channel);
} catch (InterruptedException e) {
LOG.info("failed to enqueue a request message", e);
- failureCount.incrementAndGet();
}
}
@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);
Review comment:
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);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3384: STORM-3749 improve logging for server error
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3384:
URL: https://github.com/apache/storm/pull/3384#discussion_r588780047
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/StormServerHandler.java
##########
@@ -51,14 +48,13 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
server.received(msg, channel.remoteAddress().toString(), channel);
} catch (InterruptedException e) {
LOG.info("failed to enqueue a request message", e);
- failureCount.incrementAndGet();
}
}
@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);
Review comment:
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] bipinprasad commented on a change in pull request #3384: STORM-3749 improve logging for server error
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3384:
URL: https://github.com/apache/storm/pull/3384#discussion_r588785956
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/StormServerHandler.java
##########
@@ -51,14 +48,13 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
server.received(msg, channel.remoteAddress().toString(), channel);
} catch (InterruptedException e) {
LOG.info("failed to enqueue a request message", e);
- failureCount.incrementAndGet();
}
}
@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);
Review comment:
Ignore previous comment. I just tested this in a storm test and parameters work in LOG.error correctly as expected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] agresch merged pull request #3384: STORM-3749 improve logging for server error
Posted by GitBox <gi...@apache.org>.
agresch merged pull request #3384:
URL: https://github.com/apache/storm/pull/3384
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [storm] Ethanlm commented on a change in pull request #3384: STORM-3749 improve logging for server error
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3384:
URL: https://github.com/apache/storm/pull/3384#discussion_r588370696
##########
File path: storm-client/src/jvm/org/apache/storm/messaging/netty/StormServerHandler.java
##########
@@ -51,14 +48,13 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
server.received(msg, channel.remoteAddress().toString(), channel);
} catch (InterruptedException e) {
LOG.info("failed to enqueue a request message", e);
- failureCount.incrementAndGet();
}
}
@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);
Review comment:
change "to" to "from" since this is an `ChannelInboundHandlerAdapter`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org