madrob commented on a change in pull request #1963:
URL: https://github.com/apache/lucene-solr/pull/1963#discussion_r503332224
##########
File path: solr/core/src/java/org/apache/solr/core/XmlConfigFile.java
##########
@@ -145,15 +139,15 @@ public XmlConfigFile(SolrResourceLoader loader, String
name, InputSource is, Str
db.setErrorHandler(xmllog);
try {
doc = db.parse(is);
- origDoc = copyDoc(doc);
+ origDoc = doc;
Review comment:
If these are always the same, down still need two copies of it?
##########
File path: solr/core/src/java/org/apache/solr/cloud/CloudConfigSetService.java
##########
@@ -39,14 +43,28 @@
*/
public class CloudConfigSetService extends ConfigSetService {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
-
+ private Map<String, ConfigCacheEntry> cache = new ConcurrentHashMap<>();
Review comment:
nit: make final?
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.common;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Predicate;
+
+import org.apache.solr.cluster.api.SimpleMap;
+
+/**
+ * A generic interface that represents a config file, mostly XML
+ */
+public interface ConfigNode {
+ ThreadLocal<Function<String,String>> SUBSTITUTES = new ThreadLocal<>();
+
+ /**
+ * Name of the tag
+ */
+ String name();
+
+ /**
+ * Text value of the node
+ */
+ String textValue();
+
+ /**
+ * Attributes
+ */
+ SimpleMap<String> attributes();
+
+ /**
+ * Child by name
+ */
+ default ConfigNode child(String name) {
+ return child(null, name);
+ }
+
+ /**Iterate through child nodes with the name and return the first child that
matches
+ */
+ default ConfigNode child(Predicate<ConfigNode> test, String name) {
Review comment:
I think these are more natural with the order of the parameters
reversed. It also aligns better with the single argument version calling
`child(name, null)` - easier to reason about in an IDE autocomplete.
`child(name, test)` reads more similarly to the previous XPath
`//name[test]`.
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.common;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Predicate;
+
+import org.apache.solr.cluster.api.SimpleMap;
+
+/**
+ * A generic interface that represents a config file, mostly XML
+ */
+public interface ConfigNode {
+ ThreadLocal<Function<String,String>> SUBSTITUTES = new ThreadLocal<>();
+
+ /**
+ * Name of the tag
+ */
+ String name();
+
+ /**
+ * Text value of the node
+ */
+ String textValue();
+
+ /**
+ * Attributes
+ */
+ SimpleMap<String> attributes();
+
+ /**
+ * Child by name
+ */
+ default ConfigNode child(String name) {
+ return child(null, name);
+ }
+
+ /**Iterate through child nodes with the name and return the first child that
matches
+ */
+ default ConfigNode child(Predicate<ConfigNode> test, String name) {
+ ConfigNode[] result = new ConfigNode[1];
+ forEachChild(it -> {
+ if (name!=null && !name.equals(it.name())) return Boolean.TRUE;
+ if (test == null || test.test(it)) {
+ result[0] = it;
+ return Boolean.FALSE;
+ }
+ return Boolean.TRUE;
+ });
+ return result[0];
+ }
+
+ /**Iterate through child nodes with the names and return all the matching
children
+ * @param nodeNames names of tags to be returned
+ * @param test check for the nodes to be returned
+ */
+ default List<ConfigNode> children(Predicate<ConfigNode> test, String...
nodeNames) {
+ return children(test, nodeNames == null ? Collections.emptySet() :
Set.of(nodeNames));
+ }
+
+ /**Iterate through child nodes with the names and return all the matching
children
+ * @param matchNames names of tags to be returned
+ * @param test check for the nodes to be returned
+ */
+ default List<ConfigNode> children(Predicate<ConfigNode> test, Set<String>
matchNames) {
Review comment:
I would reverse this argument order too.
##########
File path: solr/solrj/src/java/org/apache/solr/common/util/DOMUtil.java
##########
@@ -37,9 +39,24 @@
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 Map<String,String> toMap(NamedNodeMap attrs) {
return toMapExcept(attrs);
}
+ public static Map<String,String> toMap(ConfigNode node) {
+ return toMapExcept(node);
+ }
+ public static Map<String,String> toMapExcept(ConfigNode node, String...
exclusions) {
Review comment:
I wish this were a Set instead of an array, but I see the parallel to
the other implementation of this.
##########
File path: solr/core/src/java/org/apache/solr/schema/FieldTypePluginLoader.java
##########
@@ -254,9 +253,9 @@ private Analyzer readAnalyzer(Node node) throws
XPathExpressionException {
("[schema.xml] analyzer/charFilter", CharFilterFactory.class, false,
false) {
@Override
- @SuppressWarnings({"rawtypes"})
- protected CharFilterFactory create(SolrClassLoader loader, String name,
String className, Node node) throws Exception {
- final Map<String,String> params = DOMUtil.toMap(node.getAttributes());
+ @SuppressWarnings("rawtypes")
Review comment:
We can remove this and a few of the other suppressions.
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.common;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Predicate;
+
+import org.apache.solr.cluster.api.SimpleMap;
+
+/**
+ * A generic interface that represents a config file, mostly XML
+ */
+public interface ConfigNode {
+ ThreadLocal<Function<String,String>> SUBSTITUTES = new ThreadLocal<>();
+
+ /**
+ * Name of the tag
+ */
+ String name();
+
+ /**
+ * Text value of the node
+ */
+ String textValue();
+
+ /**
+ * Attributes
+ */
+ SimpleMap<String> attributes();
+
+ /**
+ * Child by name
+ */
+ default ConfigNode child(String name) {
+ return child(null, name);
+ }
+
+ /**Iterate through child nodes with the name and return the first child that
matches
+ */
+ default ConfigNode child(Predicate<ConfigNode> test, String name) {
+ ConfigNode[] result = new ConfigNode[1];
+ forEachChild(it -> {
+ if (name!=null && !name.equals(it.name())) return Boolean.TRUE;
+ if (test == null || test.test(it)) {
+ result[0] = it;
+ return Boolean.FALSE;
+ }
+ return Boolean.TRUE;
+ });
+ return result[0];
+ }
+
+ /**Iterate through child nodes with the names and return all the matching
children
+ * @param nodeNames names of tags to be returned
+ * @param test check for the nodes to be returned
+ */
+ default List<ConfigNode> children(Predicate<ConfigNode> test, String...
nodeNames) {
Review comment:
This is never used, let's not add new API methods that we don't need
##########
File path: solr/solrj/src/java/org/apache/solr/common/ConfigNode.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.common;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Predicate;
+
+import org.apache.solr.cluster.api.SimpleMap;
+
+/**
+ * A generic interface that represents a config file, mostly XML
+ */
+public interface ConfigNode {
+ ThreadLocal<Function<String,String>> SUBSTITUTES = new ThreadLocal<>();
+
+ /**
+ * Name of the tag
+ */
+ String name();
+
+ /**
+ * Text value of the node
+ */
+ String textValue();
+
+ /**
+ * Attributes
+ */
+ SimpleMap<String> attributes();
+
+ /**
+ * Child by name
+ */
+ default ConfigNode child(String name) {
Review comment:
I think this is a place where returning `Optional<ConfigNode>` makes
sense, to align ourselves with JDK stream `findFirst`-like methods.
##########
File path:
solr/core/src/java/org/apache/solr/util/plugin/AbstractPluginLoader.java
##########
@@ -135,15 +134,13 @@ protected T create(SolrClassLoader loader, String name,
String className, Node n
* If a default element is defined, it will be returned from this function.
*
*/
- public T load(SolrClassLoader loader, NodeList nodes )
Review comment:
Does this have to be a list? `Iterable<? extends ConfigNode>`?
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
