-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean up thread context map implementations #2690
Conversation
Before merging this please ensure you look into the issues I had with the performance tests and StringArrayThreadContextMap. |
570a1fa
to
fc51ccd
Compare
I can not reproduce the issues in this branch. I suspect that they were due to the changes in the The performance is similar to the
The poor performance when the context map has 50 or more entries is due to the Large context maps and the |
After reversing the lookup order in
|
Unless we find some problems, I will merge this PR next week. A reduced version of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitting my initial review. I liked the simplification in general.
log4j-api/src/main/java/org/apache/logging/log4j/internal/map/UnmodifiableArrayBackedMap.java
Outdated
Show resolved
Hide resolved
log4j-api/src/main/java/org/apache/logging/log4j/spi/DefaultThreadContextMap.java
Outdated
Show resolved
Hide resolved
log4j-perf-test/src/main/java/org/apache/logging/log4j/perf/appender/StringAppender.java
Outdated
Show resolved
Hide resolved
...ite/antora/modules/ROOT/partials/manual/systemproperties/properties-thread-context-core.adoc
Outdated
Show resolved
Hide resolved
- Make `StringArray` the default thread context map - Remove `CopyOnWriteSortedArrayThreadContextMap` - Move `GarbageFreeSortedArrayThreadContextMap` to `log4j-core`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only reviewed functional and documentation changes, they LGTM. (Did not have time to check the tests.)
8a43fa3
to
f27f6bc
Compare
Thanks, I am merging it. |
We bring some order in the current variety of
ThreadContextMap
implementations by:DefaultThreadContextMap
with the more performantStringArrayThreadContextMap
introduced in Implement faster ThreadContextMap #2330,CopyOnWriteSortedArrayThreadContextMap
, since it has no advantages over the default one. It doesn't even guarantee garbage-free logging.GarbageFreeSortedArrayThreadContextMap
tolog4j-core
, since no other logging implementation can profit from it.The documentation has been updated to include the fact that the configuration options for
ThreadContext
actually depend on the underlying implementation:ThreadContext
configuration options,log4j2.simplelogShowContextMap
andlog4j2.isThreadContextMapInheritable
options,MDC
,This PR deprecates #2593.