abhinav-phi commented on PR #4341:
URL: https://github.com/apache/logging-log4j2/pull/4341#issuecomment-5901023387
Thanks for the review — all three comments addressed in `26694b9`.
**1. Cover the flush derivation** — added
`testUnbufferedIoForcesImmediateFlush` in `ConsoleAppenderTest.java:187`, as
suggested:
```java
@Test
void testUnbufferedIoForcesImmediateFlush() {
final ConsoleAppender app = ConsoleAppender.newBuilder()
.setName("testUnbufferedIoForcesImmediateFlush")
.setBufferedIo(false)
.setImmediateFlush(false)
.build();
try {
assertTrue(app.getImmediateFlush());
} finally {
app.stop();
}
}
```
I also checked it actually guards the derivation rather than just passing:
reverting `ConsoleAppender.java:251` to `isImmediateFlush()` makes this one
test fail (`expected: <true> but was: <false>`) while the other ten in the
class stay green. Reverted afterwards.
**2. Drop the `bufferSize` warning** — removed, you were right on both
counts. `Constants.ENCODER_BYTE_BUFFER_SIZE` is `8 * 1024`, so the condition
was true for every `bufferedIo="false"` console that never set `bufferSize`;
and the message was untrue anyway, since the manager still receives
`bufferSize` in that case.
**3. Remove `bufferedIo` from the manager name** — done,
`ConsoleAppender.java:242` is now:
```java
final String managerName = target.name() + '.' + follow + '.' + direct + '.'
+ bufferSize;
```
Your reasoning checks out: `FactoryData` only carries `(os, name, layout,
bufferSize)`, and `createManager` builds `new OutputStreamManager(data.os,
data.name, data.layout, true, data.bufferSize)` — `bufferedIo` never reaches
the manager, it only feeds the appender's flush flag.
One extra thing worth mentioning: `mvn spotless:check` was already failing
on the previous head in these two same files (a wrapped `getDefaultManager`
call and an over-long `assertEquals` in `testDefaultBufferSize`). I ran the
project's own `spotless:apply`, so formatting is now clean — the change is
whitespace-only.
Verification: `ConsoleAppenderTest` 11/11, plus
`ConsoleAppenderBuilderTest`, `AbstractAppenderBuilderTest` and
`PropertiesConfigurationTest` all green, and `spotless:check` reports 0 files
needing changes.
Side note if you see it in CI: `Configurator1Test` fails 20/21 locally with
`expected: <X> but was: <Null>`, but I confirmed it fails identically on the
unmodified `d93c9dc`, so it looks environmental and unrelated to this PR.
Ready for another look — happy to rebase on `2.x` if it has moved.
--
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]
