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/06/03 15:18:03 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_r644843888
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigOverlay.java
##########
@@ -182,16 +181,24 @@ public static boolean isEditableProp(String path, boolean isXpath, List<String>
}
- public static Class<?> checkEditable(String path, boolean isXpath, List<String> hierarchy) {
- List<String> parts = StrUtils.splitSmart(path, isXpath ? '/' : '.');
+
+ @SuppressWarnings({"rawtypes"})
Review comment:
I just went through and fixed a lot of these, please do not re-introduce them.
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigOverlay.java
##########
@@ -182,16 +181,24 @@ public static boolean isEditableProp(String path, boolean isXpath, List<String>
}
- public static Class<?> checkEditable(String path, boolean isXpath, List<String> hierarchy) {
- List<String> parts = StrUtils.splitSmart(path, isXpath ? '/' : '.');
+
+ @SuppressWarnings({"rawtypes"})
+ public static Class checkEditable(String path, boolean isXpath, List<String> hierarchy) {
return isEditable(isXpath, hierarchy, StrUtils.splitSmart(path, isXpath ? '/' : '.'));
}
- private static Class<?> isEditable(boolean isXpath, List<String> hierarchy, List<String> parts) {
+ @SuppressWarnings("rawtypes")
+ private static Class isEditable(boolean isXpath, List<String> hierarchy, List<String> parts) {
+
Object obj = editable_prop_map;
for (int i = 0; i < parts.size(); i++) {
String part = parts.get(i);
- boolean isAttr = isXpath && part.startsWith("@");
+ boolean isAttr = false;
+ try {
+ isAttr = isXpath && part.startsWith("@");
+ } catch (RuntimeException e) {
+ throw e;
+ }
Review comment:
Does this do anything?
##########
File path: solr/core/src/java/org/apache/solr/handler/DumpRequestHandler.java
##########
@@ -36,6 +36,7 @@
public class DumpRequestHandler extends RequestHandlerBase
{
+
Review comment:
?
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -36,23 +43,69 @@
*/
String name();
- /**
- * Text value of the node
- */
- String textValue();
-
/**
* Attributes
*/
SimpleMap<String> attributes();
- /**
+ /**N
Review comment:
?
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -36,23 +43,69 @@
*/
String name();
- /**
- * Text value of the node
- */
- String textValue();
-
/**
* Attributes
*/
SimpleMap<String> attributes();
- /**
+ /**N
* Child by name
*/
default ConfigNode child(String name) {
return child(null, name);
}
+ /**
+ * Child by name or return an empty node if null
+ * if there are multiple values , it returns the first elem
+ * This never returns a null
+ */
+ default ConfigNode get(String name) {
+ ConfigNode child = child(null, name);
+ return child == null? EMPTY: child;
+ }
+ default ConfigNode get(String name, Predicate<ConfigNode> test) {
+ List<ConfigNode> children = getAll(test, name);
+ if(children.isEmpty()) return EMPTY;
+ return children.get(0);
+ }
+ default ConfigNode get(String name, int idx) {
+ List<ConfigNode> children = getAll(null, name);
+ if(idx < children.size()) return children.get(idx);
+ return EMPTY;
+
+ }
+
+ default ConfigNode child(List<String> path) {
Review comment:
I think this is unused
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -564,7 +641,7 @@ public HttpCachingConfig getHttpCachingConfig() {
"cacheControl", cacheControlHeader);
}
- public static enum LastModFrom {
+ public enum LastModFrom {
Review comment:
why?
##########
File path: solr/core/src/java/org/apache/solr/core/PluginInfo.java
##########
@@ -143,6 +156,17 @@ public PluginInfo(String type, Map<String,Object> map) {
isFromSolrConfig = true;
}
+ private List<PluginInfo> loadSubPlugins(ConfigNode node) {
+ List<PluginInfo> children = new ArrayList<>();
+ //if there is another sub tag with a non namedlist tag that has to be another plugin
+ node.forEachChild(nd -> {
+ if (NL_TAGS.contains(nd.name())) return null;
+ PluginInfo pluginInfo = new PluginInfo(nd, null, false, false);
+ if (pluginInfo.isEnabled()) children.add(pluginInfo);
+ return null;
+ });
+ return children.isEmpty() ? Collections.<PluginInfo>emptyList() : unmodifiableList(children);
Review comment:
`Collections.emptyList()` is enough, don't need an explicit type there
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSetService.java
##########
@@ -435,4 +436,10 @@ protected NamedList loadConfigSetFlags(CoreDescriptor cd, SolrResourceLoader loa
*/
public abstract List<String> getAllConfigFiles(String configName) throws IOException;
+ public interface ConfigResource {
Review comment:
Where is this used?
##########
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:
please configure your editor to not make wildcard imports.
##########
File path: solr/solrj/src/java/org/apache/solr/common/util/WrappedSimpleMap.java
##########
@@ -46,4 +47,14 @@ public WrappedSimpleMap(Map<String, T> delegate) {
this.delegate = delegate;
}
+ @Override
+ public Map<String, T> asMap(Map<String, T> sink) {
+ sink.putAll(delegate);
+ return sink;
+ }
+
+ @Override
Review comment:
nit: indentation
##########
File path: solr/solrj/src/java/org/apache/solr/cluster/api/SimpleMap.java
##########
@@ -82,4 +84,13 @@ public void accept(String k, T v) {
default void writeMap(EntryWriter ew) throws IOException {
forEachEntry(ew::putNoEx);
}
+
+ default Map<String, T> asMap( Map<String, T> sink) {
Review comment:
nit: whitespace
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigOverlay.java
##########
@@ -182,16 +181,24 @@ public static boolean isEditableProp(String path, boolean isXpath, List<String>
}
- public static Class<?> checkEditable(String path, boolean isXpath, List<String> hierarchy) {
- List<String> parts = StrUtils.splitSmart(path, isXpath ? '/' : '.');
+
+ @SuppressWarnings({"rawtypes"})
+ public static Class checkEditable(String path, boolean isXpath, List<String> hierarchy) {
return isEditable(isXpath, hierarchy, StrUtils.splitSmart(path, isXpath ? '/' : '.'));
}
- private static Class<?> isEditable(boolean isXpath, List<String> hierarchy, List<String> parts) {
+ @SuppressWarnings("rawtypes")
Review comment:
Same as above.
##########
File path: solr/solrj/src/java/org/apache/solr/common/util/DOMUtil.java
##########
@@ -39,7 +41,7 @@
public static final String XML_RESERVED_PREFIX = "xml";
- public static final Set<String> NL_TAGS = Set.of("str", "int","long","float","double","bool");
+ public static final Set<String> NL_TAGS = new HashSet<>(Arrays.asList("str", "int", "long", "float", "double", "bool", "null"));
Review comment:
Does this need to be a mutable set? Why?
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -90,15 +143,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());
+
+ class Helpers {
+ static boolean _bool(Object v, boolean def) { return v == null ? def : Boolean.parseBoolean(v.toString()); }
+ static String _txt(Object v, String def) { return v == null ? def : v.toString(); }
+ static int _int(Object v, int def) { return v==null? def: Integer.parseInt(v.toString()); }
+ static double _double(Object v, double def) { return v == null ? def: Double.parseDouble(v.toString()); }
+ public static Predicate<ConfigNode> at(int i) {
Review comment:
unused
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -36,23 +43,69 @@
*/
String name();
- /**
- * Text value of the node
- */
- String textValue();
-
/**
* Attributes
*/
SimpleMap<String> attributes();
- /**
+ /**N
* Child by name
*/
default ConfigNode child(String name) {
return child(null, name);
}
+ /**
+ * Child by name or return an empty node if null
+ * if there are multiple values , it returns the first elem
+ * This never returns a null
+ */
+ default ConfigNode get(String name) {
+ ConfigNode child = child(null, name);
+ return child == null? EMPTY: child;
+ }
+ default ConfigNode get(String name, Predicate<ConfigNode> test) {
+ List<ConfigNode> children = getAll(test, name);
+ if(children.isEmpty()) return EMPTY;
+ return children.get(0);
+ }
+ default ConfigNode get(String name, int idx) {
+ List<ConfigNode> children = getAll(null, name);
+ if(idx < children.size()) return children.get(idx);
+ return EMPTY;
+
+ }
+
+ default ConfigNode child(List<String> path) {
+ ConfigNode node = this;
+ for (String s : path) {
+ node = node.child(s);
+ if (node == null) break;
+ }
+ return node;
+ }
+
+ default ConfigNode child(String name, Supplier<RuntimeException> err) {
+ ConfigNode n = child(name);
+ if(n == null) throw err.get();
+ return n;
+ }
+
+ default boolean boolVal(boolean def) { return _bool(txt(),def); }
+ default int intVal(int def) { return _int(txt(), def); }
+ default String attr(String name, String def) { return _txt(attributes().get(name), def);}
+ default String attr(String name) { return attributes().get(name);}
+ default String requiredStrAttr(String name, Supplier<RuntimeException> err) {
+ String attr = attr(name);
+ if(attr == null && err != null) throw err.get();
+ return attr;
+ }
+ default int intAttr(String name, int def) { return _int(attr(name), def); }
+ default boolean boolAttr(String name, boolean def){ return _bool(attr(name), def); }
+ default String txt(String def) { return txt() == null ? def : txt();}
+ default String textValue() {return txt();}
+ String txt() ;
Review comment:
Let's leave the method for subclasses to implement as `textValue` and have the other methods refer to it instead of forcing subclasses to refactor as well.
##########
File path: solr/core/src/java/org/apache/solr/util/DOMConfigNode.java
##########
@@ -54,7 +56,8 @@ public DOMConfigNode(Node node) {
@Override
public SimpleMap<String> attributes() {
if (attrs != null) return attrs;
- return attrs = new WrappedSimpleMap<>(DOMUtil.toMap(node.getAttributes()));
+ Map<String, String> attrs = DOMUtil.toMap(node.getAttributes());
+ return this.attrs = attrs.size() == 0 ? EMPTY : new WrappedSimpleMap<>(attrs);
Review comment:
This is very difficult to read, can we split it into multiple statements?
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchema.java
##########
@@ -730,12 +730,11 @@ protected void postReadInform() {
* Loads the copy fields
*/
protected synchronized void loadCopyFields(ConfigNode n) {
- List<ConfigNode> nodes = n.children(COPY_FIELD);
+ List<ConfigNode> nodes = n.getAll(COPY_FIELD);
ConfigNode f = n.child(FIELDS);
if (f != null) {
- List<ConfigNode> c = f.children(COPY_FIELD);
- if (nodes.isEmpty()) nodes = c;
- else nodes.addAll(c);
+ nodes = new ArrayList<>(nodes);
Review comment:
Why do we make a copy here, but at line 653 above modify the returned value directly?
##########
File path: solr/core/src/test/org/apache/solr/core/TestBadConfig.java
##########
@@ -26,14 +28,17 @@ public void testNRTModeProperty() throws Exception {
assertConfigs("bad-solrconfig-nrtmode.xml","schema.xml", "nrtMode");
}
+ @Ignore
Review comment:
Why are these getting ignored? At a minimum, they should get an AwaitsFix with a JIRA link
##########
File path: solr/core/src/resources/EditableSolrConfigAttributes.json
##########
@@ -56,10 +56,6 @@
"enableLazyFieldLoading":1,
"boolTofilterOptimizer":1,
"maxBooleanClauses":1},
- "jmx":{
Review comment:
Why did this go away?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -158,6 +166,27 @@ public static SolrConfig readFromResourceLoader(SolrResourceLoader loader, Strin
throw new SolrException(ErrorCode.SERVER_ERROR, "Error loading solr config from " + resource, e);
}
}
+ private class ResourceProvider implements Function<String, InputStream> {
+ int zkVersion;
+ int hash = -1;
+ InputStream in;
+ String fileName;
+
+ ResourceProvider(InputStream in) {
+ this.in = in;
+ if (in instanceof ZkSolrResourceLoader.ZkByteArrayInputStream) {
+ ZkSolrResourceLoader.ZkByteArrayInputStream zkin = (ZkSolrResourceLoader.ZkByteArrayInputStream) in;
+ zkVersion = zkin.getStat().getVersion();
+ hash = Objects.hash(zkVersion, overlay.getZnodeVersion());
+ this.fileName = zkin.fileName;
+ }
+ }
+
+ @Override
+ public InputStream apply(String s) {
+ return in;
+ }
Review comment:
Why do we implement Function if we don't use the argument? And always return the same input stream? Where does this get called from?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -372,9 +446,19 @@ public static final Version parseLuceneVersionString(final String matchVersion)
public final Class<?> clazz;
public final String tag;
public final Set<PluginOpts> options;
+ final Function<SolrConfig, List<ConfigNode>> configReader;
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ private SolrPluginInfo(Class clz, String tag, PluginOpts... opts) {
+ this(solrConfig -> solrConfig.root.getAll(null, tag), clz, tag, opts);
+
+ }
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ private SolrPluginInfo(Function<SolrConfig, List<ConfigNode>> configReader, Class clz, String tag, PluginOpts... opts) {
+ this.configReader = configReader;
Review comment:
please fix this usage of rawtypes
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -585,18 +662,20 @@ public static LastModFrom parse(final String s) {
private final String cacheControlHeader;
private final Long maxAge;
private final LastModFrom lastModFrom;
+ private ConfigNode configNode;
private HttpCachingConfig(SolrConfig conf) {
+ configNode = conf.root;
- never304 = conf.getBool(CACHE_PRE + "@never304", false);
+ //"requestDispatcher/httpCaching/";
+ never304 = get("requestDispatcher").get("httpCaching").boolAttr("never304", false);
- etagSeed = conf.get(CACHE_PRE + "@etagSeed", "Solr");
+ etagSeed = get("requestDispatcher").get("httpCaching").attr("etagSeed", "Solr");
- lastModFrom = LastModFrom.parse(conf.get(CACHE_PRE + "@lastModFrom",
- "openTime"));
+ lastModFrom = LastModFrom.parse(get("requestDispatcher").get("httpCaching").attr("lastModFrom","openTime"));
- cacheControlHeader = conf.get(CACHE_PRE + "cacheControl", null);
+ cacheControlHeader = get("requestDispatcher").get("httpCaching").get("cacheControl").txt();
Review comment:
store get(requestDispatcher).get(httpCaching) to a local variable?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -426,15 +510,15 @@ public static ConfigOverlay getConfigOverlay(SolrResourceLoader loader) {
}
protected UpdateHandlerInfo loadUpdatehandlerInfo() {
- return new UpdateHandlerInfo(get("updateHandler/@class", null),
- getInt("updateHandler/autoCommit/maxDocs", -1),
- getInt("updateHandler/autoCommit/maxTime", -1),
- convertHeapOptionStyleConfigStringToBytes(get("updateHandler/autoCommit/maxSize", "")),
- getBool("updateHandler/indexWriter/closeWaitsForMerges", true),
- getBool("updateHandler/autoCommit/openSearcher", true),
- getInt("updateHandler/autoSoftCommit/maxDocs", -1),
- getInt("updateHandler/autoSoftCommit/maxTime", -1),
- getBool("updateHandler/commitWithin/softCommit", true));
+ return new UpdateHandlerInfo( get("updateHandler").attr("class"),
+ get("updateHandler").get("autoCommit").get("maxDocs").intVal( -1),
+ get("updateHandler").get("autoCommit").get("maxTime").intVal( -1),
+ convertHeapOptionStyleConfigStringToBytes(get("updateHandler").get("autoCommit").get("maxSize").txt()),
+ get("updateHandler").get("indexWriter").get("closeWaitsForMerges").boolVal(true),
+ get("updateHandler").get("autoCommit").get("openSearcher").boolVal(true),
+ get("updateHandler").get("autoSoftCommit").get("maxDocs").intVal(-1),
+ get("updateHandler").get("autoSoftCommit").get("maxTime").intVal(-1),
+ get("updateHandler").get("commitWithin").get("softCommit").boolVal(true));
Review comment:
should we store the result of get(updateHandler) so we're not calling it 8 times?
##########
File path: solr/core/src/java/org/apache/solr/search/CacheConfig.java
##########
@@ -88,36 +84,34 @@ public void setRegenerator(CacheRegenerator regenerator) {
this.regenerator = regenerator;
}
- public static Map<String, CacheConfig> getMultipleConfigs(SolrConfig solrConfig, String configPath) {
- NodeList nodes = (NodeList) solrConfig.evaluate(configPath, XPathConstants.NODESET);
- if (nodes == null || nodes.getLength() == 0) return new LinkedHashMap<>();
- Map<String, CacheConfig> result = new HashMap<>(nodes.getLength());
- for (int i = 0; i < nodes.getLength(); i++) {
- Node node = nodes.item(i);
- if ("true".equals(DOMUtil.getAttrOrDefault(node, "enabled", "true"))) {
- CacheConfig config = getConfig(solrConfig, node.getNodeName(),
- DOMUtil.toMap(node.getAttributes()), configPath);
+ public static Map<String, CacheConfig> getMultipleConfigs(SolrConfig solrConfig, String configPath, List<ConfigNode> nodes) {
+ if (nodes == null || nodes.size() == 0) return new LinkedHashMap<>();
+ Map<String, CacheConfig> result = new HashMap<>(nodes.size());
+ for (int i = 0; i < nodes.size(); i++) {
+ ConfigNode node = nodes.get(i);
+ if (node.boolAttr("enabled", true)) {
+ CacheConfig config = getConfig(solrConfig, node.name(), node.attributes().asMap(), configPath);
result.put(config.args.get(NAME), config);
}
}
return result;
}
- public static CacheConfig getConfig(SolrConfig solrConfig, String xpath) {
- Node node = solrConfig.getNode(xpath, false);
- if(node == null || !"true".equals(DOMUtil.getAttrOrDefault(node, "enabled", "true"))) {
- Map<?, ?> m = solrConfig.getOverlay().getEditableSubProperties(xpath);
- if(m==null) return null;
+ @SuppressWarnings({"unchecked"})
+ public static CacheConfig getConfig(SolrConfig solrConfig, ConfigNode node, String xpath) {
+ if (!node.exists() || !"true".equals(node.attributes().get("enabled", "true"))) {
+ Map<String, Object> m = solrConfig.getOverlay().getEditableSubProperties(xpath);
+ if (m == null) return null;
List<String> parts = StrUtils.splitSmart(xpath, '/');
- return getConfig(solrConfig,parts.get(parts.size()-1), Collections.emptyMap(), xpath);
+ return getConfig(solrConfig, parts.get(parts.size() - 1), Collections.EMPTY_MAP, xpath);
Review comment:
please keep the usage of the type safe emptyMap instead of EMPTY_MAP and remove the suppress warning annotation
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -168,140 +197,180 @@ public static SolrConfig readFromResourceLoader(SolrResourceLoader loader, Strin
* @param isConfigsetTrusted false if configset was uploaded using unsecured configset upload API, true otherwise
* @param substitutableProperties optional properties to substitute into the XML
*/
+ @SuppressWarnings("unchecked")
private SolrConfig(SolrResourceLoader loader, String name, boolean isConfigsetTrusted, Properties substitutableProperties)
- throws ParserConfigurationException, IOException, SAXException {
- // insist we have non-null substituteProperties; it might get overlayed
- super(loader, name, null, "/config/", substitutableProperties == null ? new Properties() : substitutableProperties);
+ throws IOException {
+ this.resourceLoader = loader;
+ this.resourceName = name;
+ this.substituteProperties = substitutableProperties;
getOverlay();//just in case it is not initialized
- getRequestParams();
- initLibs(loader, isConfigsetTrusted);
- luceneMatchVersion = SolrConfig.parseLuceneVersionString(getVal(IndexSchema.LUCENE_MATCH_VERSION_PARAM, true));
- log.info("Using Lucene MatchVersion: {}", luceneMatchVersion);
-
- String indexConfigPrefix;
-
- // Old indexDefaults and mainIndex sections are deprecated and fails fast for luceneMatchVersion=>LUCENE_4_0_0.
- // For older solrconfig.xml's we allow the old sections, but never mixed with the new <indexConfig>
- boolean hasDeprecatedIndexConfig = (getNode("indexDefaults", false) != null) || (getNode("mainIndex", false) != null);
- if (hasDeprecatedIndexConfig) {
- throw new SolrException(ErrorCode.FORBIDDEN, "<indexDefaults> and <mainIndex> configuration sections are discontinued. Use <indexConfig> instead.");
- } else {
- indexConfigPrefix = "indexConfig";
- }
- assertWarnOrFail("The <nrtMode> config has been discontinued and NRT mode is always used by Solr." +
- " This config will be removed in future versions.", getNode(indexConfigPrefix + "/nrtMode", false) == null,
- true
- );
- assertWarnOrFail("Solr no longer supports forceful unlocking via the 'unlockOnStartup' option. "+
- "This is no longer necessary for the default lockType except in situations where "+
- "it would be dangerous and should not be done. For other lockTypes and/or "+
- "directoryFactory options it may also be dangerous and users must resolve "+
- "problematic locks manually.",
- null == getNode(indexConfigPrefix + "/unlockOnStartup", false),
- true // 'fail' in trunk
- );
-
- // Parse indexConfig section, using mainIndex as backup in case old config is used
- indexConfig = new SolrIndexConfig(this, "indexConfig", null);
-
- booleanQueryMaxClauseCount = getInt("query/maxBooleanClauses", IndexSearcher.getMaxClauseCount());
- if (IndexSearcher.getMaxClauseCount() < booleanQueryMaxClauseCount) {
- log.warn("solrconfig.xml: <maxBooleanClauses> of {} is greater than global limit of {} {}"
- , booleanQueryMaxClauseCount, IndexSearcher.getMaxClauseCount()
- , "and will have no effect set 'maxBooleanClauses' in solr.xml to increase global limit");
- }
-
- // Warn about deprecated / discontinued parameters
- // boolToFilterOptimizer has had no effect since 3.1
- if (get("query/boolTofilterOptimizer", null) != null)
- log.warn("solrconfig.xml: <boolTofilterOptimizer> is currently not implemented and has no effect.");
- if (get("query/HashDocSet", null) != null)
- log.warn("solrconfig.xml: <HashDocSet> is deprecated and no longer used.");
+ // insist we have non-null substituteProperties; it might get overlaid
+ Map<String, IndexSchemaFactory.VersionedConfig> configCache =null;
+ if (loader.getCoreContainer() != null && loader.getCoreContainer().getObjectCache() != null) {
+ configCache = (Map<String, IndexSchemaFactory.VersionedConfig>) loader.getCoreContainer().getObjectCache()
+ .computeIfAbsent(ConfigSetService.ConfigResource.class.getName(), s -> new ConcurrentHashMap<>());
+ ResourceProvider rp = new ResourceProvider(loader.openResource(name));
+ IndexSchemaFactory.VersionedConfig cfg = rp.fileName == null ? null : configCache.get(rp.fileName);
+ if (cfg != null) {
+ if (rp.hash != -1) {
+ if (rp.hash == cfg.version) {
+ log.debug("LOADED_FROM_CACHE");
+ root = cfg.data;
+ } else {
+ readXml(loader, name, configCache, rp);
+ }
+ }
+ }
+ }
+ if(root == null) {
+ readXml(loader, name, configCache,new ResourceProvider(loader.openResource(name)) );
+ }
+ ConfigNode.SUBSTITUTES.set(key -> {
+ if (substitutableProperties != null && substitutableProperties.containsKey(key)) {
+ return substitutableProperties.getProperty(key);
+ } else {
+ Object o = overlay.getUserProps().get(key);
+ return o == null ? null : o.toString();
+ }
+ });
+ try {
+ getRequestParams();
+ initLibs(loader, isConfigsetTrusted);
+ String val = root.child(IndexSchema.LUCENE_MATCH_VERSION_PARAM,
+ () -> new RuntimeException("Missing: " + IndexSchema.LUCENE_MATCH_VERSION_PARAM)).txt();
+
+ luceneMatchVersion = SolrConfig.parseLuceneVersionString(val);
+ log.info("Using Lucene MatchVersion: {}", luceneMatchVersion);
+
+ String indexConfigPrefix;
+
+ // Old indexDefaults and mainIndex sections are deprecated and fails fast for luceneMatchVersion=>LUCENE_4_0_0.
+ // For older solrconfig.xml's we allow the old sections, but never mixed with the new <indexConfig>
+ boolean hasDeprecatedIndexConfig = get("indexDefaults").exists() || get("mainIndex").exists();
+ if (hasDeprecatedIndexConfig) {
+ throw new SolrException(ErrorCode.FORBIDDEN, "<indexDefaults> and <mainIndex> configuration sections are discontinued. Use <indexConfig> instead.");
+ } else {
+ indexConfigPrefix = "indexConfig";
+ }
+ assertWarnOrFail("The <nrtMode> config has been discontinued and NRT mode is always used by Solr." +
+ " This config will be removed in future versions.", get(indexConfigPrefix).get("nrtMode").isNull(),
+ true
+ );
+ assertWarnOrFail("Solr no longer supports forceful unlocking via the 'unlockOnStartup' option. " +
+ "This is no longer necessary for the default lockType except in situations where " +
+ "it would be dangerous and should not be done. For other lockTypes and/or " +
+ "directoryFactory options it may also be dangerous and users must resolve " +
+ "problematic locks manually.",
+ !get(indexConfigPrefix).get("unlockOnStartup").exists(),
+ true // 'fail' in trunk
+ );
+
+ // Parse indexConfig section, using mainIndex as backup in case old config is used
+ indexConfig = new SolrIndexConfig(get("indexConfig"), null);
+
+ booleanQueryMaxClauseCount = get("query").get("maxBooleanClauses").intVal(BooleanQuery.getMaxClauseCount());
+ if (IndexSearcher.getMaxClauseCount() < booleanQueryMaxClauseCount) {
+ log.warn("solrconfig.xml: <maxBooleanClauses> of {} is greater than global limit of {} and will have no effect {}"
+ , booleanQueryMaxClauseCount, BooleanQuery.getMaxClauseCount()
+ , "set 'maxBooleanClauses' in solr.xml to increase global limit");
+ }
Review comment:
Should be consistently IndexSearcher.maxClauseCount
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -168,140 +197,180 @@ public static SolrConfig readFromResourceLoader(SolrResourceLoader loader, Strin
* @param isConfigsetTrusted false if configset was uploaded using unsecured configset upload API, true otherwise
* @param substitutableProperties optional properties to substitute into the XML
*/
+ @SuppressWarnings("unchecked")
private SolrConfig(SolrResourceLoader loader, String name, boolean isConfigsetTrusted, Properties substitutableProperties)
- throws ParserConfigurationException, IOException, SAXException {
- // insist we have non-null substituteProperties; it might get overlayed
- super(loader, name, null, "/config/", substitutableProperties == null ? new Properties() : substitutableProperties);
+ throws IOException {
+ this.resourceLoader = loader;
+ this.resourceName = name;
+ this.substituteProperties = substitutableProperties;
getOverlay();//just in case it is not initialized
- getRequestParams();
- initLibs(loader, isConfigsetTrusted);
- luceneMatchVersion = SolrConfig.parseLuceneVersionString(getVal(IndexSchema.LUCENE_MATCH_VERSION_PARAM, true));
- log.info("Using Lucene MatchVersion: {}", luceneMatchVersion);
-
- String indexConfigPrefix;
-
- // Old indexDefaults and mainIndex sections are deprecated and fails fast for luceneMatchVersion=>LUCENE_4_0_0.
- // For older solrconfig.xml's we allow the old sections, but never mixed with the new <indexConfig>
- boolean hasDeprecatedIndexConfig = (getNode("indexDefaults", false) != null) || (getNode("mainIndex", false) != null);
- if (hasDeprecatedIndexConfig) {
- throw new SolrException(ErrorCode.FORBIDDEN, "<indexDefaults> and <mainIndex> configuration sections are discontinued. Use <indexConfig> instead.");
- } else {
- indexConfigPrefix = "indexConfig";
- }
- assertWarnOrFail("The <nrtMode> config has been discontinued and NRT mode is always used by Solr." +
- " This config will be removed in future versions.", getNode(indexConfigPrefix + "/nrtMode", false) == null,
- true
- );
- assertWarnOrFail("Solr no longer supports forceful unlocking via the 'unlockOnStartup' option. "+
- "This is no longer necessary for the default lockType except in situations where "+
- "it would be dangerous and should not be done. For other lockTypes and/or "+
- "directoryFactory options it may also be dangerous and users must resolve "+
- "problematic locks manually.",
- null == getNode(indexConfigPrefix + "/unlockOnStartup", false),
- true // 'fail' in trunk
- );
-
- // Parse indexConfig section, using mainIndex as backup in case old config is used
- indexConfig = new SolrIndexConfig(this, "indexConfig", null);
-
- booleanQueryMaxClauseCount = getInt("query/maxBooleanClauses", IndexSearcher.getMaxClauseCount());
- if (IndexSearcher.getMaxClauseCount() < booleanQueryMaxClauseCount) {
- log.warn("solrconfig.xml: <maxBooleanClauses> of {} is greater than global limit of {} {}"
- , booleanQueryMaxClauseCount, IndexSearcher.getMaxClauseCount()
- , "and will have no effect set 'maxBooleanClauses' in solr.xml to increase global limit");
- }
-
- // Warn about deprecated / discontinued parameters
- // boolToFilterOptimizer has had no effect since 3.1
- if (get("query/boolTofilterOptimizer", null) != null)
- log.warn("solrconfig.xml: <boolTofilterOptimizer> is currently not implemented and has no effect.");
- if (get("query/HashDocSet", null) != null)
- log.warn("solrconfig.xml: <HashDocSet> is deprecated and no longer used.");
+ // insist we have non-null substituteProperties; it might get overlaid
+ Map<String, IndexSchemaFactory.VersionedConfig> configCache =null;
+ if (loader.getCoreContainer() != null && loader.getCoreContainer().getObjectCache() != null) {
+ configCache = (Map<String, IndexSchemaFactory.VersionedConfig>) loader.getCoreContainer().getObjectCache()
+ .computeIfAbsent(ConfigSetService.ConfigResource.class.getName(), s -> new ConcurrentHashMap<>());
Review comment:
Introducing a cache here seems like it is part of a different change set.
##########
File path: solr/core/src/java/org/apache/solr/search/CacheConfig.java
##########
@@ -88,36 +84,34 @@ public void setRegenerator(CacheRegenerator regenerator) {
this.regenerator = regenerator;
}
- public static Map<String, CacheConfig> getMultipleConfigs(SolrConfig solrConfig, String configPath) {
- NodeList nodes = (NodeList) solrConfig.evaluate(configPath, XPathConstants.NODESET);
- if (nodes == null || nodes.getLength() == 0) return new LinkedHashMap<>();
- Map<String, CacheConfig> result = new HashMap<>(nodes.getLength());
- for (int i = 0; i < nodes.getLength(); i++) {
- Node node = nodes.item(i);
- if ("true".equals(DOMUtil.getAttrOrDefault(node, "enabled", "true"))) {
- CacheConfig config = getConfig(solrConfig, node.getNodeName(),
- DOMUtil.toMap(node.getAttributes()), configPath);
+ public static Map<String, CacheConfig> getMultipleConfigs(SolrConfig solrConfig, String configPath, List<ConfigNode> nodes) {
+ if (nodes == null || nodes.size() == 0) return new LinkedHashMap<>();
+ Map<String, CacheConfig> result = new HashMap<>(nodes.size());
+ for (int i = 0; i < nodes.size(); i++) {
Review comment:
improve this to be a foreach loop
##########
File path: solr/core/src/test/org/apache/solr/core/TestConfLoadPerf.java
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.io.InputStream;
+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();
+
+
+ File file = new File(configSetDir, "solrconfig.xml");
+ byte[] b = new byte[(int) file.length()];
+ new FileInputStream(file).read(b);
Review comment:
Use Files.readAllBytes
--
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.
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