SOLR-14680: Remove SimpleMap (affects NamedList) #2856
Conversation
and the rest of SolrJ org.apache.solr.cluster.api NamedList: * new getOrDefault * remove forEachEntry, forEachKey, abortableForEachKey, abortableForEach * remove asMap, get(key, default)
github-actions
bot
added
client:solrj
tests
cat:search
cat:cloud
cat:index
cat:cli
cat:schema
labels
| } | ||
|
|
||
| public String subtituteVal(String s) { | ||
| private static String substituteVal(String s) { | ||
| return PropertiesUtil.substitute(s, SUBSTITUTES.get()); |
There was a problem hiding this 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!
There was a problem hiding this comment.
This was probbaly done to avoid changing the public APIs. We can change them now, not a big deal
There was a problem hiding this comment.
re were some Map conversions (via asMap which surprisingly creates a new Map;
YEs. it has to be changed. But asMap is not used in many places
There was a problem hiding this comment.
Okay; I'll file a pair of JIRA issues tomorrow if you don't first:
- Replace ConfigNode.SUBSTITUTES (ThreadLocal); redesign
- Rename NamedList.asMap to something like toMapRecursively
|
I don't think I have the knowledge to weigh in here, but I did like that this removes 10x more lines than it adds! |
# Conflicts: # solr/solrj-zookeeper/src/java/org/apache/solr/common/LazySolrCluster.java
| Map<String, Object> systemInfo = | ||
| solrClient | ||
| .request(new GenericSolrRequest(SolrRequest.METHOD.GET, CommonParams.SYSTEM_INFO_PATH)) | ||
| .asMap(); |
There was a problem hiding this comment.
needless asMap call. @epugh you added this code a year ago, maybe because you don't like NamedList?
There was a problem hiding this comment.
You are right that I don't like NamedList, it's always been a weird Solr specific data structure. Why did we feel the need to come up with a unique data structure that isn't in the JDK? Having said that, what you are proposing is clearly an improvement!
There was a problem hiding this comment.
I also wish we just had a cleaner data structure for these GSR that return JSON. I love that in our tests we have assertJQ and JQ methods that lets us grab some json. In a perfect world, this call would have returned a JSON datastructure, and I would use a Json query line to pluck out the core_root. This would be more compelling if we had a much more nested Map data structure to go through!
There was a problem hiding this comment.
I think @gerlowskija would argue that the "perfect world" (we're slowly iterating towards) would not be JSON at this level of abstraction, it'd be a a machine-generated POJO generated from our OpenAPI that you call normal typed methods on. Correct me if I'm wrong Jason; I agree with that vision too.
But in the meantime Eric, get to know NamedList. It's not going anywhere anytime soon, and it's also not hard to use IMO. I wish it had the convenience methods that SolrParams has but not a big deal.
| } | ||
|
|
||
| public String subtituteVal(String s) { | ||
| private static String substituteVal(String s) { | ||
| return PropertiesUtil.substitute(s, SUBSTITUTES.get()); |
There was a problem hiding this comment.
Okay; I'll file a pair of JIRA issues tomorrow if you don't first:
- Replace ConfigNode.SUBSTITUTES (ThreadLocal); redesign
- Rename NamedList.asMap to something like toMapRecursively
|
If I should break this up, or file another JIRA, let me know. The SimpleMap part feels kind of separate, especially since it impacts the ubiquitous NamedList. |
|
I plan to merge this next week with the following CHANGES.txt under 9.8 Don't plan to say anything in 10 since removing deprecated code doesn't deserve a mention, generally. |
and remove the rest of SolrJ org.apache.solr.cluster.api
NamedList:
https://issues.apache.org/jira/browse/SOLR-14680
This was a pain because SimpleMap was a little more entrenched. It deserves a CHANGES.txt calling out briefly that NamedList is impacted, since this is so omnipresent in Solr.