You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/11/02 16:37:24 UTC
[GitHub] [solr] madrob commented on a change in pull request #160: SOLR-15337: Avoid XPath in solrconfig.xml parsing
madrob commented on a change in pull request #160:
URL: https://github.com/apache/solr/pull/160#discussion_r741248212
##########
File path: solr/core/src/java/org/apache/solr/util/DataConfigNode.java
##########
@@ -19,64 +19,72 @@
import java.util.ArrayList;
import java.util.Collections;
-import java.util.HashMap;
+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 com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
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 {
- final String name;
- final SimpleMap<String> attributes;
- private final Map<String, List<ConfigNode>> kids = new HashMap<>();
- private final String textData;
+ public final String name;
+ public final SimpleMap<String> attributes;
+ public final SimpleMap<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());
- textData = root.textValue();
+ textData = root.txt();
root.forEachChild(it -> {
List<ConfigNode> nodes = kids.computeIfAbsent(it.name(),
k -> new ArrayList<>());
-
- nodes.add(new DataConfigNode(it));
+ nodes.add(new DataConfigNode(it));
return Boolean.TRUE;
});
-
+ for (Map.Entry<String, List<ConfigNode>> e : kids.entrySet()) {
+ if(e.getValue() != null) {
+ e.setValue(ImmutableList.copyOf(e.getValue()));
Review comment:
prefer `List.copyOf`
##########
File path: solr/core/src/java/org/apache/solr/util/DataConfigNode.java
##########
@@ -19,64 +19,72 @@
import java.util.ArrayList;
import java.util.Collections;
-import java.util.HashMap;
+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 com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
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 {
- final String name;
- final SimpleMap<String> attributes;
- private final Map<String, List<ConfigNode>> kids = new HashMap<>();
- private final String textData;
+ public final String name;
+ public final SimpleMap<String> attributes;
+ public final SimpleMap<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());
- textData = root.textValue();
+ textData = root.txt();
root.forEachChild(it -> {
List<ConfigNode> nodes = kids.computeIfAbsent(it.name(),
k -> new ArrayList<>());
-
- nodes.add(new DataConfigNode(it));
+ nodes.add(new DataConfigNode(it));
return Boolean.TRUE;
});
-
+ for (Map.Entry<String, List<ConfigNode>> e : kids.entrySet()) {
+ if(e.getValue() != null) {
+ e.setValue(ImmutableList.copyOf(e.getValue()));
+ }
+ }
+ this.kids = kids.isEmpty()? EMPTY: new WrappedSimpleMap<>(ImmutableMap.copyOf(kids));
Review comment:
prefer `Map.copyOf`
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -90,15 +133,76 @@ default ConfigNode child(Predicate<ConfigNode> test, String name) {
return result;
}
- default List<ConfigNode> children(String name) {
- return children(null, Collections.singleton(name));
+ default List<ConfigNode> getAll(String name) {
+ return getAll(null, Collections.singleton(name));
}
+ default boolean exists() { return true; }
+ default boolean isNull() { return false; }
+
/** abortable iterate through children
*
* @param fun consume the node and return true to continue or false to abort
*/
void forEachChild(Function<ConfigNode, Boolean> fun);
+ /**An empty node object.
+ * usually returned when the node is absent
+ *
+ */
+ ConfigNode EMPTY = new ConfigNode() {
+ @Override
+ public String name() {
+ return null;
+ }
+
+ @Override
+ public String txt() { return null; }
+
+ @Override
+ public SimpleMap<String> attributes() {
+ return empty_attrs;
+ }
+
+ @Override
+ public String attr(String name) { return null; }
+
+ @Override
+ public String attr(String name, String def) { return def; }
+ @Override
+ public ConfigNode child(String name) { return null; }
+
+ @Override
+ public ConfigNode get(String name) {
+ return EMPTY;
+ }
+
+ public boolean exists() { return false; }
Review comment:
nit: missing override
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -17,16 +17,23 @@
package org.apache.solr.common;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
+import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Predicate;
+import java.util.function.Supplier;
import org.apache.solr.cluster.api.SimpleMap;
+import org.apache.solr.common.util.WrappedSimpleMap;
+
+import static org.apache.solr.common.ConfigNode.Helpers.*;
Review comment:
this was never addressed.
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -90,15 +133,76 @@ default ConfigNode child(Predicate<ConfigNode> test, String name) {
return result;
}
- default List<ConfigNode> children(String name) {
- return children(null, Collections.singleton(name));
+ default List<ConfigNode> getAll(String name) {
+ return getAll(null, Collections.singleton(name));
}
+ default boolean exists() { return true; }
+ default boolean isNull() { return false; }
+
/** abortable iterate through children
*
* @param fun consume the node and return true to continue or false to abort
*/
void forEachChild(Function<ConfigNode, Boolean> fun);
+ /**An empty node object.
Review comment:
nit: malformed javadoc
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -90,15 +133,76 @@ default ConfigNode child(Predicate<ConfigNode> test, String name) {
return result;
}
- default List<ConfigNode> children(String name) {
- return children(null, Collections.singleton(name));
+ default List<ConfigNode> getAll(String name) {
+ return getAll(null, Collections.singleton(name));
}
+ default boolean exists() { return true; }
+ default boolean isNull() { return false; }
+
/** abortable iterate through children
*
* @param fun consume the node and return true to continue or false to abort
*/
void forEachChild(Function<ConfigNode, Boolean> fun);
+ /**An empty node object.
+ * usually returned when the node is absent
+ *
+ */
+ ConfigNode EMPTY = new ConfigNode() {
+ @Override
+ public String name() {
+ return null;
+ }
+
+ @Override
+ public String txt() { return null; }
+
+ @Override
+ public SimpleMap<String> attributes() {
+ return empty_attrs;
+ }
+
+ @Override
+ public String attr(String name) { return null; }
+
+ @Override
+ public String attr(String name, String def) { return def; }
+ @Override
+ public ConfigNode child(String name) { return null; }
+
+ @Override
+ public ConfigNode get(String name) {
+ return EMPTY;
+ }
+
+ public boolean exists() { return false; }
+
+ @Override
+ public boolean isNull() { return true; }
+
+ @Override
+ public void forEachChild(Function<ConfigNode, Boolean> fun) { }
+ } ;
+ SimpleMap<String> empty_attrs = new WrappedSimpleMap<>(Collections.emptyMap());
Review comment:
duplicated with DataConfigNode
##########
File path: solr/core/src/test/org/apache/solr/core/TestConfLoadPerf.java
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.core;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cloud.ZkSolrResourceLoader;
+import org.apache.solr.common.util.SuppressForbidden;
+import org.apache.solr.util.ExternalPaths;
+import org.apache.zookeeper.data.Stat;
+import org.junit.Ignore;
+
+import static org.apache.solr.core.TestConfigSets.solrxml;
+
+public class TestConfLoadPerf extends SolrTestCaseJ4 {
+
+ @Ignore
+ @SuppressForbidden(reason = "Needed to provide time for tests.")
+ public void testPerf() throws Exception{
+ String sourceHome = ExternalPaths.SOURCE_HOME;
+ File configSetDir = new File(sourceHome, "server/solr/configsets/sample_techproducts_configs/conf");
+
+ String configSetsBaseDir = TEST_PATH().resolve("configsets").toString();
+ byte[] b = Files.readAllBytes(new File(configSetDir, "solrconfig.xml").toPath());
+ Path testDirectory = createTempDir();
+
+ System.setProperty("configsets", configSetsBaseDir);
+
+ CoreContainer container = new CoreContainer(SolrXmlConfig.fromString(testDirectory, solrxml));
+ container.load();
+ container.shutdown();
+
+ SolrResourceLoader srl = new SolrResourceLoader("temp", Collections.emptyList(), container.solrHome, container.getResourceLoader().classLoader){
+
+ @Override
+ public CoreContainer getCoreContainer() {
+ return container;
+ }
+
+ @Override
+ public InputStream openResource(String resource) throws IOException {
+ if(resource.equals("solrconfig.xml")) {
+ Stat stat = new Stat();
+ stat.setVersion(1);
+ return new ZkSolrResourceLoader.ZkByteArrayInputStream(b, new File(configSetDir, "solrconfig.xml").getAbsolutePath(), stat);
+ } else {
+ throw new FileNotFoundException(resource);
+ }
+
+ }
+ };
+ System.gc();
+ long heapSize = Runtime.getRuntime().totalMemory();
+ List<SolrConfig> allConfigs = new ArrayList<>();
+ long startTime = System.currentTimeMillis();
+ for(int i=0;i<100;i++) {
+ allConfigs.add(SolrConfig.readFromResourceLoader(srl, "solrconfig.xml", true, null));
+
+ }
+ System.gc();
+ System.out.println("TIME_TAKEN : "+(System.currentTimeMillis()-startTime));
+ System.out.println("HEAP_SIZE : "+((Runtime.getRuntime().totalMemory()-heapSize)/(1024)));
Review comment:
What do we expect here? When I ran it, I got:
```
TIME_TAKEN : 261
HEAP_SIZE : 0
```
Is that good? Is that correct?
##########
File path: solr/core/src/java/org/apache/solr/update/SolrIndexConfig.java
##########
@@ -108,88 +109,78 @@ private SolrIndexConfig(SolrConfig solrConfig) {
// enable coarse-grained metrics by default
metricsInfo = new PluginInfo("metrics", Collections.emptyMap(), null, null);
}
-
+ private ConfigNode get(String s) { return node.get(s); }
+ public SolrIndexConfig(SolrConfig cfg, SolrIndexConfig def) {
+ this(cfg.get("indexConfig"), def);
+ }
/**
* Constructs a SolrIndexConfig which parses the Lucene related config params in solrconfig.xml
- * @param solrConfig the overall SolrConfig object
- * @param prefix the XPath prefix for which section to parse (mandatory)
* @param def a SolrIndexConfig instance to pick default values from (optional)
*/
- public SolrIndexConfig(SolrConfig solrConfig, String prefix, SolrIndexConfig def) {
- if (prefix == null) {
- prefix = "indexConfig";
- log.debug("Defaulting to prefix '{}' for index configuration", prefix);
- }
-
+ public SolrIndexConfig(ConfigNode cfg, SolrIndexConfig def) {
+ this.node = cfg;
if (def == null) {
- def = new SolrIndexConfig(solrConfig);
+ def = new SolrIndexConfig();
}
+
// sanity check: this will throw an error for us if there is more then one
// config section
- Object unused = solrConfig.getNode(prefix, false);
+// Object unused = solrConfig.getNode(prefix, false);
// Assert that end-of-life parameters or syntax is not in our config.
// Warn for luceneMatchVersion's before LUCENE_3_6, fail fast above
assertWarnOrFail("The <mergeScheduler>myclass</mergeScheduler> syntax is no longer supported in solrconfig.xml. Please use syntax <mergeScheduler class=\"myclass\"/> instead.",
- !((solrConfig.getNode(prefix + "/mergeScheduler", false) != null) && (solrConfig.get(prefix + "/mergeScheduler/@class", null) == null)),
+ get("mergeScheduler").isNull() || get("mergeScheduler").attr("class") != null,
true);
assertWarnOrFail("Beginning with Solr 7.0, <mergePolicy>myclass</mergePolicy> is no longer supported, use <mergePolicyFactory> instead.",
- !((solrConfig.getNode(prefix + "/mergePolicy", false) != null) && (solrConfig.get(prefix + "/mergePolicy/@class", null) == null)),
+ get("mergePolicy").isNull() || get("mergePolicy").attr("class") != null,
true);
assertWarnOrFail("The <luceneAutoCommit>true|false</luceneAutoCommit> parameter is no longer valid in solrconfig.xml.",
- solrConfig.get(prefix + "/luceneAutoCommit", null) == null,
+ get("luceneAutoCommit").isNull(),
true);
- useCompoundFile = solrConfig.getBool(prefix+"/useCompoundFile", def.useCompoundFile);
- maxBufferedDocs = solrConfig.getInt(prefix+"/maxBufferedDocs", def.maxBufferedDocs);
- ramBufferSizeMB = solrConfig.getDouble(prefix+"/ramBufferSizeMB", def.ramBufferSizeMB);
- maxCommitMergeWaitMillis = solrConfig.getInt(prefix+"/maxCommitMergeWaitTime", def.maxCommitMergeWaitMillis);
+ useCompoundFile = get("useCompoundFile").boolVal(def.useCompoundFile);
+ maxBufferedDocs = get("maxBufferedDocs").intVal(def.maxBufferedDocs);
+ ramBufferSizeMB = get("ramBufferSizeMB").doubleVal(def.ramBufferSizeMB);
+ maxCommitMergeWaitMillis = get("maxCommitMergeWaitTime").intVal(def.maxCommitMergeWaitMillis);
// how do we validate the value??
- ramPerThreadHardLimitMB = solrConfig.getInt(prefix+"/ramPerThreadHardLimitMB", def.ramPerThreadHardLimitMB);
+ ramPerThreadHardLimitMB = get("ramPerThreadHardLimitMB").intVal(def.ramPerThreadHardLimitMB);
- writeLockTimeout=solrConfig.getInt(prefix+"/writeLockTimeout", def.writeLockTimeout);
- lockType=solrConfig.get(prefix+"/lockType", def.lockType);
+ writeLockTimeout= get("writeLockTimeout").intVal(def.writeLockTimeout);
+ lockType = get("lockType").txt(def.lockType);
- List<PluginInfo> infos = solrConfig.readPluginInfos(prefix + "/metrics", false, false);
- if (infos.isEmpty()) {
- metricsInfo = def.metricsInfo;
- } else {
- metricsInfo = infos.get(0);
- }
- mergeSchedulerInfo = getPluginInfo(prefix + "/mergeScheduler", solrConfig, def.mergeSchedulerInfo);
- mergePolicyFactoryInfo = getPluginInfo(prefix + "/mergePolicyFactory", solrConfig, def.mergePolicyFactoryInfo);
+ metricsInfo = getPluginInfo(get("metrics"), def.metricsInfo);
+ mergeSchedulerInfo = getPluginInfo(get("mergeScheduler"), def.mergeSchedulerInfo);
+ mergePolicyFactoryInfo = getPluginInfo(get("mergePolicyFactory"), def.mergePolicyFactoryInfo);
assertWarnOrFail("Beginning with Solr 7.0, <mergePolicy> is no longer supported, use <mergePolicyFactory> instead.",
- getPluginInfo(prefix + "/mergePolicy", solrConfig, null) == null,
+ get("mergePolicy").isNull(),
true);
assertWarnOrFail("Beginning with Solr 7.0, <maxMergeDocs> is no longer supported, configure it on the relevant <mergePolicyFactory> instead.",
- solrConfig.getInt(prefix+"/maxMergeDocs", 0) == 0,
+ get("maxMergeDocs").isNull(),
true);
assertWarnOrFail("Beginning with Solr 7.0, <mergeFactor> is no longer supported, configure it on the relevant <mergePolicyFactory> instead.",
- solrConfig.getInt(prefix+"/mergeFactor", 0) == 0,
+ get("maxMergeFactor").isNull(),
Review comment:
did you mean to rename this?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org