ramanathan1504 commented on code in PR #4341:
URL: https://github.com/apache/logging-log4j2/pull/4341#discussion_r4125887233
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/ConsoleAppender.java:
##########
@@ -236,16 +237,22 @@ public ConsoleAppender build() {
? getDirectOutputStream(target)
: follow ? getFollowOutputStream(target) :
getDefaultOutputStream(target);
- final String managerName = target.name() + '.' + follow + '.' +
direct;
- final OutputStreamManager manager =
- OutputStreamManager.getManager(managerName, new
FactoryData(stream, managerName, layout), factory);
+ final boolean bufferedIo = isBufferedIo();
+ final int bufferSize = getBufferSize();
+ if (!bufferedIo && bufferSize > 0) {
+ LOGGER.warn("The bufferSize is set to {} but bufferedIo is
false.", bufferSize);
+ }
Review Comment:
`bufferSize` defaults to 8192, so this warns for every `bufferedIo="false"`
console, even one that never sets `bufferSize`. The Console manager still uses
`bufferSize` in that case, so the message is also not true here. Can we drop it?
```suggestion
```
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/ConsoleAppenderTest.java:
##########
@@ -154,6 +156,51 @@ void testDefaultAppenderImmediateFlush() {
}
}
+ @Test
+ void testBufferSizeHonored() {
+ final ConsoleAppender app = ConsoleAppender.newBuilder()
+ .setName("testBufferSizeHonored")
+ .setBufferSize(16384)
+ .build();
+ try {
+ assertNotNull(app.getManager());
+ assertEquals(16384, app.getManager().getByteBuffer().capacity());
+ } finally {
+ app.stop();
+ }
+ }
+
+ @Test
+ void testDefaultBufferSize() {
+ final ConsoleAppender app =
+
ConsoleAppender.newBuilder().setName("testDefaultBufferSize").build();
+ try {
+ assertEquals(Constants.ENCODER_BYTE_BUFFER_SIZE,
app.getManager().getByteBuffer().capacity());
+ } finally {
+ app.stop();
+ }
+ }
Review Comment:
Nothing covers the flush derivation. This test fails if `!bufferedIo ||
isImmediateFlush()` goes back to `isImmediateFlush()`.
```suggestion
}
@Test
void testUnbufferedIoForcesImmediateFlush() {
final ConsoleAppender app = ConsoleAppender.newBuilder()
.setName("testUnbufferedIoForcesImmediateFlush")
.setBufferedIo(false)
.setImmediateFlush(false)
.build();
try {
assertTrue(app.getImmediateFlush());
} finally {
app.stop();
}
}
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/ConsoleAppender.java:
##########
@@ -236,16 +237,22 @@ public ConsoleAppender build() {
? getDirectOutputStream(target)
: follow ? getFollowOutputStream(target) :
getDefaultOutputStream(target);
- final String managerName = target.name() + '.' + follow + '.' +
direct;
- final OutputStreamManager manager =
- OutputStreamManager.getManager(managerName, new
FactoryData(stream, managerName, layout), factory);
+ final boolean bufferedIo = isBufferedIo();
+ final int bufferSize = getBufferSize();
+ if (!bufferedIo && bufferSize > 0) {
+ LOGGER.warn("The bufferSize is set to {} but bufferedIo is
false.", bufferSize);
+ }
+ final String managerName =
+ target.name() + '.' + follow + '.' + direct + '.' +
bufferedIo + '.' + bufferSize;
Review Comment:
The manager does not change with `bufferedIo`. Only the appender's flush
flag does. Keeping it in the name splits identical managers.
```suggestion
final String managerName = target.name() + '.' + follow + '.' +
direct + '.' + bufferSize;
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
