Skip to content
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.

Sign up for GitHub

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

SOLR-14680: Remove SimpleMap (affects NamedList) #2856

Merged
merged 9 commits into from
Dec 10, 2024
Merged

SOLR-14680: Remove SimpleMap (affects NamedList) #2856

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8596dfc
SOLR-14680: Remove SimpleMap (affects NamedList)
dsmiley Nov 9, 2024
5e8a5c5
forEach
dsmiley Nov 9, 2024
6b3f5d0
Mostly reinstate some asMap calls that should have been that.
dsmiley Nov 13, 2024
0fba181
Merge branch 'main' into noSimpleMap
dsmiley Nov 13, 2024
6370577
Fix SpatialHeatmapFacetsTest
dsmiley Nov 13, 2024
92c1662
Merge branch 'refs/heads/main' into fork/dsmiley/noSimpleMap
dsmiley Dec 9, 2024
71c0ed1
move CLIUtils change after the refactor
dsmiley Dec 9, 2024
f1017f6
Un-delete but deprecate the files
dsmiley Dec 10, 2024
24efa78
Keep NamedList implementing SimpleMap in 9x
dsmiley Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/cli/ApiTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ protected String callGet(String url, String credentials) throws Exception {
NamedList<Object> response = solrClient.request(req);
// pretty-print the response to stdout
CharArr arr = new CharArr();
new JSONWriter(arr, 2).write(response.asMap());
new JSONWriter(arr, 2).write(response.asShallowMap());
dsmiley marked this conversation as resolved.
Show resolved Hide resolved
return arr.toString();
}
}
Expand Down
10 changes: 4 additions & 6 deletions solr/core/src/java/org/apache/solr/cli/CreateTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import org.apache.commons.cli.CommandLine;
Expand Down Expand Up @@ -167,10 +166,9 @@ protected void createCore(CommandLine cli, SolrClient solrClient) throws Excepti

String coreRootDirectory; // usually same as solr home, but not always

Map<String, Object> systemInfo =
solrClient
.request(new GenericSolrRequest(SolrRequest.METHOD.GET, CommonParams.SYSTEM_INFO_PATH))
.asMap();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needless asMap call. @epugh you added this code a year ago, maybe because you don't like NamedList?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the POJO! And yep.

NamedList<?> systemInfo =
solrClient.request(
new GenericSolrRequest(SolrRequest.METHOD.GET, CommonParams.SYSTEM_INFO_PATH));

// convert raw JSON into user-friendly output
coreRootDirectory = (String) systemInfo.get("core_root");
Expand Down Expand Up @@ -321,7 +319,7 @@ protected void createCollection(CloudSolrClient cloudSolrClient, CommandLine cli
if (isVerbose()) {
// pretty-print the response to stdout
CharArr arr = new CharArr();
new JSONWriter(arr, 2).write(response.asMap());
new JSONWriter(arr, 2).write(response.asShallowMap());
echo(arr.toString());
}
String endMessage =
Expand Down
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/cli/DeleteTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ protected void deleteCollection(CloudSolrClient cloudSolrClient, CommandLine cli
if (isVerbose() && response != null) {
// pretty-print the response to stdout
CharArr arr = new CharArr();
new JSONWriter(arr, 2).write(response.asMap());
new JSONWriter(arr, 2).write(response.asShallowMap());
echo(arr.toString());
echo("\n");
}
Expand Down
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/cli/SolrCLI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ public static boolean safeCheckCoreExists(String solrUrl, String coreName, Strin
Map<String, Object> failureStatus =
(Map<String, Object>) existsCheckResult.get("initFailures");
String errorMsg = (String) failureStatus.get(coreName);
final boolean hasName = coreStatus != null && coreStatus.asMap().containsKey(NAME);
final boolean hasName = coreStatus != null && coreStatus.get(NAME) != null;
exists = hasName || errorMsg != null;
wait = hasName && errorMsg == null && "true".equals(coreStatus.get("isLoading"));
} while (wait && System.nanoTime() - startWaitAt < MAX_WAIT_FOR_CORE_LOAD_NANOS);
Expand Down
5 changes: 3 additions & 2 deletions solr/core/src/java/org/apache/solr/cli/StatusTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,13 +358,14 @@ protected Map<String, String> getCloudStatus(SolrClient solrClient, String zkHos
Map<String, String> cloudStatus = new LinkedHashMap<>();
cloudStatus.put("ZooKeeper", (zkHost != null) ? zkHost : "?");

// TODO add booleans to request just what we want; not everything
NamedList<Object> json = solrClient.request(new CollectionAdminRequest.ClusterStatus());

List<String> liveNodes = (List<String>) json.findRecursive("cluster", "live_nodes");
cloudStatus.put("liveNodes", String.valueOf(liveNodes.size()));

Map<String, Object> collections =
((NamedList<Object>) json.findRecursive("cluster", "collections")).asMap();
// TODO get this as a metric from the metrics API instead, or something else.
var collections = (NamedList<Object>) json.findRecursive("cluster", "collections");
cloudStatus.put("collections", String.valueOf(collections.size()));

return cloudStatus;
Expand Down
4 changes: 2 additions & 2 deletions solr/core/src/java/org/apache/solr/core/OverlaidConfigNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.function.Function;
import java.util.function.Predicate;
import org.apache.solr.cluster.api.SimpleMap;
import org.apache.solr.common.ConfigNode;

/** A config node impl which has an overlay */
Expand Down Expand Up @@ -80,7 +80,7 @@ public String name() {
}

@Override
public SimpleMap<String> attributes() {
public Map<String, String> attributes() {
return delegate.attributes();
}

Expand Down
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/core/PluginInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ public PluginInfo(ConfigNode node, String err, boolean requireName, boolean requ
className = cName.className;
pkgName = cName.pkg;
initArgs = DOMUtil.childNodesToNamedList(node);
attributes = node.attributes().asMap();
attributes = node.attributes();
children = loadSubPlugins(node);
isFromSolrConfig = true;
}
Expand Down
7 changes: 4 additions & 3 deletions solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Properties;
import java.util.Set;
import java.util.function.Consumer;
Expand Down Expand Up @@ -131,10 +132,10 @@ public static NodeConfig fromConfig(
// since it is arranged as a separate section it is placed here
Map<String, String> coreAdminHandlerActions =
readNodeListAsNamedList(root.get("coreAdminHandlerActions"), "<coreAdminHandlerActions>")
.asMap()
.asShallowMap()
.entrySet()
.stream()
.collect(Collectors.toMap(item -> item.getKey(), item -> item.getValue().toString()));
.collect(Collectors.toMap(Entry::getKey, item -> item.getValue().toString()));

UpdateShardHandlerConfig updateConfig;
if (deprecatedUpdateConfig == null) {
Expand Down Expand Up @@ -733,7 +734,7 @@ private static MetricsConfig getMetricsConfig(ConfigNode metrics) {
ConfigNode caching = metrics.get("solr/metrics/caching");
if (caching != null) {
Object threadsCachingIntervalSeconds =
DOMUtil.childNodesToNamedList(caching).get("threadsIntervalSeconds", null);
DOMUtil.childNodesToNamedList(caching).get("threadsIntervalSeconds");
builder.setCacheConfig(
new MetricsConfig.CacheConfig(
threadsCachingIntervalSeconds == null
Expand Down
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/schema/IndexSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ protected void readSchema(ConfigSetService.ConfigResource is) {
log.info("{}", sb);
}

version = Float.parseFloat(rootNode.attributes().get("version", "1.0f"));
version = Float.parseFloat(rootNode.attributes().getOrDefault("version", "1.0f"));

// load the Field Types
final FieldTypePluginLoader typeLoader =
Expand Down
4 changes: 2 additions & 2 deletions solr/core/src/java/org/apache/solr/search/CacheConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public static Map<String, CacheConfig> getMultipleConfigs(
for (ConfigNode node : nodes) {
if (node.boolAttr("enabled", true)) {
CacheConfig config =
getConfig(loader, solrConfig, node.name(), node.attributes().asMap(), configPath);
getConfig(loader, solrConfig, node.name(), node.attributes(), configPath);
result.put(config.args.get(NAME), config);
}
}
Expand All @@ -98,7 +98,7 @@ public static CacheConfig getConfig(SolrConfig solrConfig, ConfigNode node, Stri
if (!node.boolAttr("enabled", true) || !node.exists()) {
return null;
}
return getConfig(solrConfig, node.name(), node.attributes().asMap(), xpath);
return getConfig(solrConfig, node.name(), node.attributes(), xpath);
}

public static CacheConfig getConfig(
Expand Down
3 changes: 2 additions & 1 deletion solr/core/src/java/org/apache/solr/update/UpdateLog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,8 @@ public void init(PluginInfo info) {
}
int timeoutMs =
objToInt(
info.initArgs.get("docLockTimeoutMs", info.initArgs.get("versionBucketLockTimeoutMs")),
info.initArgs.getOrDefault(
"docLockTimeoutMs", info.initArgs.get("versionBucketLockTimeoutMs")),
EnvUtils.getPropertyAsLong("solr.update.docLockTimeoutMs", 0L).intValue());
updateLocks = new UpdateLocks(timeoutMs);

Expand Down
11 changes: 3 additions & 8 deletions solr/core/src/java/org/apache/solr/util/DOMConfigNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,19 @@
package org.apache.solr.util;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.function.Function;
import org.apache.solr.cluster.api.SimpleMap;
import org.apache.solr.common.ConfigNode;
import org.apache.solr.common.util.DOMUtil;
import org.apache.solr.common.util.WrappedSimpleMap;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;

/** Read using DOM */
public class DOMConfigNode implements ConfigNode {

private final Node node;
SimpleMap<String> attrs;
Map<String, String> attrs;

@Override
public String name() {
Expand All @@ -50,10 +47,10 @@ public DOMConfigNode(Node node) {
}

@Override
public SimpleMap<String> attributes() {
public Map<String, String> attributes() {
if (attrs != null) return attrs;
Map<String, String> attrs = DOMUtil.toMap(node.getAttributes());
return this.attrs = attrs.size() == 0 ? EMPTY : new WrappedSimpleMap<>(attrs);
return this.attrs = attrs.isEmpty() ? Map.of() : attrs;
}

@Override
Expand Down Expand Up @@ -85,6 +82,4 @@ public void forEachChild(Function<ConfigNode, Boolean> fun) {
if (Boolean.FALSE.equals(toContinue)) break;
}
}

private static final SimpleMap<String> EMPTY = new WrappedSimpleMap<>(Collections.emptyMap());
}
112 changes: 80 additions & 32 deletions solr/core/src/java/org/apache/solr/util/DataConfigNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,30 @@

package org.apache.solr.util;

import java.util.AbstractMap;
import java.util.AbstractSet;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.BiConsumer;
import java.util.function.Function;
import java.util.function.Predicate;
import org.apache.solr.cluster.api.SimpleMap;
import org.apache.solr.common.ConfigNode;
import org.apache.solr.common.util.PropertiesUtil;
import org.apache.solr.common.util.WrappedSimpleMap;

/** ConfigNode impl that copies and maintains data internally from DOM */
public class DataConfigNode implements ConfigNode {
public final String name;
public final SimpleMap<String> attributes;
public final SimpleMap<List<ConfigNode>> kids;
public final Map<String, String> attributes;
public final Map<String, List<ConfigNode>> kids;
public final String textData;

public DataConfigNode(ConfigNode root) {
Map<String, List<ConfigNode>> kids = new LinkedHashMap<>();
name = root.name();
attributes = wrap(root.attributes());
attributes = wrapSubstituting(root.attributes());
textData = root.txt();
root.forEachChild(
it -> {
Expand All @@ -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());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was probbaly done to avoid changing the public APIs. We can change them now, not a big deal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private SimpleMap<String> wrap(SimpleMap<String> delegate) {
/** provides a substitute view, and read-only */
private static Map<String, String> wrapSubstituting(Map<String, String> delegate) {
if (delegate.size() == 0) return delegate; // avoid unnecessary object creation
return new SimpleMap<>() {
@Override
public String get(String key) {
return subtituteVal(delegate.get(key));
}

@Override
public void forEachEntry(BiConsumer<String, ? super String> fun) {
delegate.forEachEntry((k, v) -> fun.accept(k, subtituteVal(v)));
}

@Override
public int size() {
return delegate.size();
}
};
return new SubstitutingMap(delegate);
}

@Override
Expand All @@ -88,11 +73,11 @@ public String name() {

@Override
public String txt() {
return subtituteVal(textData);
return substituteVal(textData);
}

@Override
public SimpleMap<String> attributes() {
public Map<String, String> attributes() {
return attributes;
}

Expand All @@ -104,7 +89,7 @@ public ConfigNode child(String name) {

@Override
public List<ConfigNode> getAll(String name) {
return kids.get(name, Collections.emptyList());
return kids.getOrDefault(name, List.of());
}

@Override
Expand All @@ -126,14 +111,77 @@ public List<ConfigNode> getAll(Predicate<ConfigNode> test, Set<String> matchName

@Override
public void forEachChild(Function<ConfigNode, Boolean> fun) {
kids.forEachEntry(
kids.forEach(
(s, configNodes) -> {
if (configNodes != null) {
configNodes.forEach(fun::apply);
}
});
}

public static final SimpleMap<List<ConfigNode>> EMPTY =
new WrappedSimpleMap<>(Collections.emptyMap());
private static class SubstitutingMap extends AbstractMap<String, String> {

private final Map<String, String> delegate;

SubstitutingMap(Map<String, String> delegate) {
this.delegate = delegate;
}

@Override
public String get(Object key) {
return substituteVal(delegate.get(key));
}

@Override
public int size() {
return delegate.size();
}

@Override
public Set<String> keySet() {
return delegate.keySet();
}

@Override
public Set<Entry<String, String>> entrySet() {
return new AbstractSet<>() {
@Override
public Iterator<Entry<String, String>> iterator() {
// using delegate, return an iterator using Streams
return delegate.entrySet().stream()
.map(entry -> (Entry<String, String>) new SubstitutingEntry(entry))
.iterator();
}

@Override
public int size() {
return delegate.size();
}
};
}

private static class SubstitutingEntry implements Entry<String, String> {

private final Entry<String, String> delegateEntry;

SubstitutingEntry(Entry<String, String> delegateEntry) {
this.delegateEntry = delegateEntry;
}

@Override
public String getKey() {
return delegateEntry.getKey();
}

@Override
public String getValue() {
return substituteVal(delegateEntry.getValue());
}

@Override
public String setValue(String value) {
throw new UnsupportedOperationException();
}
}
}
}
Loading
Loading