dsmiley commented on code in PR #2856:
URL: https://github.com/apache/solr/pull/2856#discussion_r1835195200
##########
solr/core/src/java/org/apache/solr/util/DataConfigNode.java:
##########
@@ -54,31 +53,17 @@ public DataConfigNode(ConfigNode root) {
e.setValue(List.copyOf(e.getValue()));
}
}
- this.kids = kids.isEmpty() ? EMPTY : new
WrappedSimpleMap<>(Map.copyOf(kids));
+ this.kids = Map.copyOf(kids);
}
- public String subtituteVal(String s) {
+ private static String substituteVal(String s) {
return PropertiesUtil.substitute(s, SUBSTITUTES.get());
Review Comment:
This particular aspect of the design of ConfigNode framework is, um, ...
really concerning to me: a ThreadLocal to hold the substitution rules.
**Wow**, so depending on which thread is *looking*(using) the map, they get
different answers?! @noblepaul can you offer an explanation as to why it's
this way instead of a normal field on the ConfigNode? This design has me
concerned with respect to this PR since there were some Map conversions (via
`asMap` which surprisingly creates a new Map; isn't a view) that maybe are now
a view; I need to check. The difference is that previously the value would be
materialized using the ThreadLocal of whoever called asMap but now would be
whoever is looking at the values. Subtle!
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
