You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/10/22 07:41:17 UTC
[GitHub] [logging-log4j2] vy opened a new pull request #591: LOG4J2-2829 SocketAppender should propagate failures when reconnection fails.
vy opened a new pull request #591:
URL: https://github.com/apache/logging-log4j2/pull/591
Certain `SocketAppender` tests are rewritten and the redundant/disabled ones are removed.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a change in pull request #591: LOG4J2-2829 SocketAppender should propagate failures when reconnection fails.
Posted by GitBox <gi...@apache.org>.
vy commented on a change in pull request #591:
URL: https://github.com/apache/logging-log4j2/pull/591#discussion_r735154155
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/net/TcpSocketManager.java
##########
@@ -238,7 +238,10 @@ protected void write(final byte[] bytes, final int offset, final int length, fin
config),
causeEx);
}
+ return;
}
+ final String message = String.format("Error writing to %s for connection %s", getName(), config);
Review comment:
Maybe it is of personal taste, but I find `var h = h(); var g = g(h); f(g)` more easy to read and debug compared to `f(g(h))`, granted there is no efficiency impact.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on pull request #591: LOG4J2-2829 SocketAppender should propagate failures when reconnection fails.
Posted by GitBox <gi...@apache.org>.
rgoers commented on pull request #591:
URL: https://github.com/apache/logging-log4j2/pull/591#issuecomment-950213698
This looks good to me.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #591: LOG4J2-2829 SocketAppender should propagate failures when reconnection fails.
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #591:
URL: https://github.com/apache/logging-log4j2/pull/591#issuecomment-950372459
@rgoers, @garydgregory, thanks so much for the review. I am merging this into `release-2.x`. Consequently I will cherry-pick this onto `master`.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #591: LOG4J2-2829 SocketAppender should propagate failures when reconnection fails.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #591:
URL: https://github.com/apache/logging-log4j2/pull/591#issuecomment-950214555
Are we losing any code coverage due to the removal of tests?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy merged pull request #591: LOG4J2-2829 SocketAppender should propagate failures when reconnection fails.
Posted by GitBox <gi...@apache.org>.
vy merged pull request #591:
URL: https://github.com/apache/logging-log4j2/pull/591
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on pull request #591: LOG4J2-2829 SocketAppender should propagate failures when reconnection fails.
Posted by GitBox <gi...@apache.org>.
vy commented on pull request #591:
URL: https://github.com/apache/logging-log4j2/pull/591#issuecomment-950371887
> Are we losing any code coverage due to the removal of tests?
Not as far as I can see. The following tests were removed: `SocketMessageLossTest`, `SocketReconnectTest`, and `SocketTest`. Except `SocketReconnectTest`, the other two were already disabled. The new `SocketAppenderReconnectTest` added covers exactly the two cases covered in `SocketReconnectTest` and performs a more elaborate check due to the corner cases subject to this PR, i.e., [LOG4J2-2829](https://issues.apache.org/jira/browse/LOG4J2-2829).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #591: LOG4J2-2829 SocketAppender should propagate failures when reconnection fails.
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #591:
URL: https://github.com/apache/logging-log4j2/pull/591#discussion_r735020059
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/net/TcpSocketManager.java
##########
@@ -238,7 +238,10 @@ protected void write(final byte[] bytes, final int offset, final int length, fin
config),
causeEx);
}
+ return;
}
+ final String message = String.format("Error writing to %s for connection %s", getName(), config);
Review comment:
Not sure we need a temp var here.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org