vy commented on code in PR #2846:
URL: https://github.com/apache/logging-log4j2/pull/2846#discussion_r1730686329
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/impl/ThreadContextDataInjector.java:
##########
@@ -87,19 +87,33 @@ private static List<ContextDataProvider>
getServiceProviders() {
return Collections.unmodifiableList(providers);
}
+ private abstract static class AbstractContextDataInjector implements
ContextDataInjector {
+
+ protected final List<ContextDataProvider> providers;
+
+ protected AbstractContextDataInjector() {
Review Comment:
Can't these two `protected` be `private` instead?
##########
src/changelog/.2.x.x/2331_custom_context_data_in_lookup.xml:
##########
@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xmlns="https://logging.apache.org/xml/ns";
+ xsi:schemaLocation="https://logging.apache.org/xml/ns
https://logging.apache.org/xml/ns/log4j-changelog-0.xsd";
+ type="added">
+ <issue id="2331" link="2330_add_faster_web_app_context_map.xml"/>
+ <description format="asciidoc">Add support for custom context data in `ctx`
lookup.</description>
Review Comment:
1. Shouldn't this rather be a `fixed`? Yes, we're _adding_ a new behaviour,
but one that was missing. Nowhere we had stated that `ctx` lookup (or any
particular component) won't work with custom `CDP`s.
2. `issue.link` needs to be fixed.
3. Could you rewrite the `description` a little bit more accurately, please?
(Suggestion: _"Fix custom thread-context data provider handling in lookups and
filters"_ – since it is not only the `ctx` lookup we fixed.)
--
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]
