Restore UNCACHED thread name updates in AsyncLogger - #4280
AndrewBystrov wants to merge 2 commits into
Conversation
|
Noted Give some time I will review and close. Thanks for the Contribution |
|
Hi @ramanathan1504, just a friendly follow-up on this PR. I understand reviews can take some time. Please let me know if there’s anything I can clarify or change on my side. Thanks! |
Surely, I will try to close this weekend.👍 |
ramanathan1504
left a comment
ramanathan1504
left a comment
There was a problem hiding this comment.
@AndrewBystrov thanks, this puts back the UNCACHED refresh that #3171 removed, and the renamed test covers the reused worker thread from the issue.
Two small points are inline on AsyncLogger.java and one on the test. @ppkarwasz can you confirm whether RingBufferLogEventTranslator#updateThreadValues should stay deprecated now that it is called again?
| } | ||
|
|
||
| private void initTranslatorThreadValues(final RingBufferLogEventTranslator translator) { | ||
| // constant check should be optimized out when using default (CACHED) |
There was a problem hiding this comment.
The default strategy is UNCACHED on current JDKs, so this comment is wrong. Can we drop it?
| // constant check should be optimized out when using default (CACHED) |
| private void initTranslatorThreadValues(final RingBufferLogEventTranslator translator) { | ||
| // constant check should be optimized out when using default (CACHED) | ||
| if (THREAD_NAME_CACHING_STRATEGY == ThreadNameCachingStrategy.UNCACHED) { | ||
| translator.updateThreadValues(); |
There was a problem hiding this comment.
updateThreadValues() is still @Deprecated, and its Javadoc says the translator is only used on the thread that created it, which is the reason this call was removed. Should the deprecation go, or should the Javadoc mention Thread.setName()?
| }); | ||
| final String firstTaskThreadName = (String) firstTask.get(); | ||
| final String secondTaskThreadName = (String) secondTask.get(); | ||
| assertNotNull(firstTaskThreadName, "firstTaskThreadName"); |
There was a problem hiding this comment.
Each task returns a string literal, so these two assertNotNull calls cannot fail. Can we remove them?
github-project-automation
Bot
moved this to Changes requested
in Log4j pull request tracker
|
Thanks for the feedback! I’ll address the requested changes. Regarding the updateThreadValues() deprecation and Javadoc, let’s wait for @ppkarwasz’s guidance before making any changes there. |
|
Thanks for the feedback! I’ve addressed the requested changes in the latest commit. For the deprecation point, I kept updateThreadValues() deprecated and updated its Javadoc to clarify its internal use after Thread.setName(String) |
3 participants
Fixes #4275.
Restore the current thread name for fully asynchronous
AsyncLoggerevents whenlog4j2.asyncLoggerThreadNameStrategyis set toUNCACHED.The thread-local translator now refreshes its thread values for the
UNCACHEDstrategy before publishing each event. TheCACHEDstrategy and the vararg translator behavior remain unchanged.The regression test submits two tasks to a single-thread executor, renames the reused worker before each log call, and verifies that both log events contain their corresponding thread names. The test fails against the base revision and passes with this fix.
The test class was renamed from
AsyncLoggerTestUncachedThreadNametoAsyncLoggerUncachedThreadNameTestbecause the old name did not match Maven Surefire's default test discovery patterns. As a result, the existing test was not included in regular test runs, despite detecting the regression when executed explicitly.Verification
AsyncLoggerThreadNameStrategyTest: 6 tests pass in both executions./mvnw verify -DskipTests -pl :log4j-core,:log4j-core-test./mvnw -N validategit diff --checkChecklist
2.xbranchsrc/changelog/.2.x.x