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 all commits
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
4 changes: 4 additions & 0 deletions solr/CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,10 @@ led to the suppression of exceptions. (Andrey Bozhko)

* SOLR-17577: Remove "solr.indexfetcher.sotimeout" system property that was for optimizing replication tests. It was disabled, but not removed. (Eric Pugh)

* SOLR-14680: NamedList: deprecating methods: forEachEntry, forEachKey, abortableForEachKey, abortableForEach,
asMap (no-arg only), get(key, default). Added getOrDefault. Deprecated the SimpleMap interface as well as the
entirety of the SolrJ package org.apache.solr.cluster.api, which wasn't used except for SimpleMap. (David Smiley)

================== 9.7.1 ==================
Bug Fixes
---------------------
Expand Down
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.asMap(10));
return arr.toString();
}
}
Expand Down
2 changes: 1 addition & 1 deletion solr/core/src/java/org/apache/solr/cli/CLIUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,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
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.asMap(10));
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.asMap(10));
echo(arr.toString());
echo("\n");
}
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 @@ -346,13 +346,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());
}
Loading
Loading