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

Reply via email to