dsmiley commented on code in PR #2126:
URL: https://github.com/apache/solr/pull/2126#discussion_r1424771268
##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -654,11 +655,28 @@ private static PluginInfo[]
getClusterSingletonPluginInfos(List<ConfigNode> node
if (nodes == null || nodes.isEmpty()) {
return new PluginInfo[0];
}
- PluginInfo[] configs = new PluginInfo[nodes.size()];
- for (int i = 0; i < nodes.size(); i++) {
- configs[i] = getPluginInfo(nodes.get(i));
+
+ List<PluginInfo> plugins =
+ nodes.stream()
+ .map(n -> new PluginInfo(n, n.name(), true, true))
+ .collect(Collectors.toList());
+
+ // Cluster plugin names must be unique
+ Set<String> names = CollectionUtil.newHashSet(nodes.size());
+ Set<String> duplicateNames =
+ plugins.stream()
+ .filter(p -> !names.add(p.name))
+ .map(p -> p.name)
+ .collect(Collectors.toSet());
+ if (!duplicateNames.isEmpty()) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "Multiple clusterSingleton sections with name '"
+ + String.join("', '", duplicateNames)
+ + "' found in solr.xml");
}
- return configs;
+
+ return plugins.toArray(new PluginInfo[nodes.size()]);
Review Comment:
our codebase stopped the practice of pre-sized; just use 0
##########
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##########
@@ -929,8 +941,8 @@ public NodeConfig build() {
metricsConfig,
cachesConfig,
tracerConfig,
- containerPluginsSource,
- containerPlugins,
+ clusterPluginsSource,
+ clusterSingletonPlugins,
Review Comment:
In NodeConfig, do we really need each supported type separated out? It's
only "clusterSingleton" now but there are others that will be added. I could
see simply an array of them or maybe a map keyed by name/type and value is the
array.
Come to think of it, I wonder if we could migrate all PluginInfo used in
NodeConfig to use this Map! This would certainly majorly cut down on
NodeConfig's Builder! If we do such a thing, it'd probably be best in a
separate PR (before or after).
##########
solr/core/src/test/org/apache/solr/api/NodeConfigClusterPluginsSourceTest.java:
##########
@@ -34,107 +31,94 @@
import org.apache.solr.common.annotation.JsonProperty;
import org.apache.solr.common.util.ReflectMapWriter;
import org.apache.solr.core.CoreContainer;
-import org.apache.solr.request.SolrQueryRequest;
-import org.apache.solr.response.SolrQueryResponse;
-import org.apache.solr.security.PermissionNameProvider;
import org.junit.BeforeClass;
import org.junit.Test;
/** Tests that verify initialization of container plugins that are declared in
solr.xml */
-public class NodeConfigContainerPluginsSourceTest extends SolrCloudTestCase {
+public class NodeConfigClusterPluginsSourceTest extends SolrCloudTestCase {
private static final String NODE_CONFIG_PLUGINS_SOURCE_XML =
- "<containerPluginsSource
class=\"org.apache.solr.api.NodeConfigContainerPluginsSource\"/>";
+ "<clusterPluginsSource
class=\"org.apache.solr.api.NodeConfigClusterPluginsSource\"/>";
+
+ // Any random value for the config parameter
+ private static int CFG_VAL;
@BeforeClass
public static void setupCluster() throws Exception {
+ CFG_VAL = random().nextInt();
configureCluster(1)
.withSolrXml(
MiniSolrCloudCluster.DEFAULT_CLOUD_SOLR_XML.replace(
"</solr>",
- NODE_CONFIG_PLUGINS_SOURCE_XML + SingletonNoConfig.xmlConfig()
+ "</solr>"))
+ NODE_CONFIG_PLUGINS_SOURCE_XML
+ + SingletonNoConfig.configXml()
+ + SingletonWithConfig.configXml(new
SingletonConfig(CFG_VAL))
+ + "</solr>"))
.addConfig(
"conf",
TEST_PATH().resolve("configsets").resolve("cloud-minimal").resolve("conf"))
.configure();
}
- /** Verifies that a cluster singleton config declared in solr.xml is loaded
into the registry */
- public void testOneClusterSingleton() {
- CoreContainer cc =
newCoreContainer(solrXml(SingletonNoConfig.xmlConfig()));
- try {
- assertEquals(1, cc.getNodeConfig().getContainerPlugins().length);
-
- ContainerPluginsRegistry registry = cc.getContainerPluginsRegistry();
- registry.refresh();
- ContainerPluginsRegistry.ApiInfo apiInfo =
registry.getPlugin(SingletonNoConfig.NAME);
- assertNotNull(apiInfo);
- assertEquals("incorrect plugin name", SingletonNoConfig.NAME,
apiInfo.getInfo().name);
- assertEquals(
- "incorrect plugin class name",
- SingletonNoConfig.class.getName(),
- apiInfo.getInfo().klass);
- assertNull(
- "config should not be set because none was specified in solr.xml",
- apiInfo.getInfo().config);
- } finally {
- cc.shutdown();
- }
- }
-
/**
- * Verifies that solr.xml allows the declaration of multiple cluster
singleton confis, and that
- * they are all loaded to the registry
+ * Verifies that the cluster singleton configs declared in solr.xml are
loaded into the registry
*/
- @Test
- public void testMultipleClusterSingletons() {
- final int cfgVal = random().nextInt();
- CoreContainer cc =
- newCoreContainer(
- solrXml(
- SingletonNoConfig.xmlConfig(),
- SingletonWithConfig.configXml(new SingletonConfig(cfgVal))));
- try {
- assertEquals(2, cc.getNodeConfig().getContainerPlugins().length);
-
- ContainerPluginsRegistry registry = cc.getContainerPluginsRegistry();
- registry.refresh();
-
- ContainerPluginsRegistry.ApiInfo apiInfo =
registry.getPlugin(SingletonNoConfig.NAME);
- assertNotNull(apiInfo);
- assertEquals(SingletonNoConfig.NAME, apiInfo.getInfo().name);
- assertEquals(SingletonNoConfig.class.getName(), apiInfo.getInfo().klass);
-
- apiInfo = registry.getPlugin(SingletonWithConfig.NAME);
- assertNotNull(apiInfo);
- assertEquals(SingletonWithConfig.NAME, apiInfo.getInfo().name);
- assertEquals(SingletonWithConfig.class.getName(),
apiInfo.getInfo().klass);
- MapWriter config = apiInfo.getInfo().config;
- Map<String, Object> configMap = new HashMap<>();
- config.toMap(configMap);
- assertEquals(cfgVal, configMap.get("cfgInt"));
- } finally {
- cc.shutdown();
- }
+ public void testClusterSingletonsRegistered() {
+
+ CoreContainer cc = cluster.getOpenOverseer().getCoreContainer();
Review Comment:
just use index 0; not the overseer. We don't care who is the overseer.
##########
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##########
@@ -929,8 +941,8 @@ public NodeConfig build() {
metricsConfig,
cachesConfig,
tracerConfig,
- containerPluginsSource,
- containerPlugins,
+ clusterPluginsSource,
+ clusterSingletonPlugins,
Review Comment:
CC @janhoy
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
