pvcnt commented on code in PR #2126:
URL: https://github.com/apache/solr/pull/2126#discussion_r1450387463
##########
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##########
@@ -211,6 +215,17 @@ private NodeConfig(
if (null == this.solrHome) throw new NullPointerException("solrHome");
if (null == this.loader) throw new NullPointerException("loader");
+ if (this.clusterPlugins != null
+ && this.clusterPlugins.length > 0
+ && !ClusterPluginsSourceConfigurator.resolveClassName()
+ .equals(NodeConfigClusterPluginsSource.class.getName())) {
+ throw new SolrException(
Review Comment:
Maybe it can be a warning instead of an exception? Not entirely sure, but
maybe people will have those in `solr.xml`, change the source later and want to
keep them. Or maybe they could be injected into the cluster API as defaults at
some point.
##########
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##########
@@ -211,6 +215,17 @@ private NodeConfig(
if (null == this.solrHome) throw new NullPointerException("solrHome");
if (null == this.loader) throw new NullPointerException("loader");
+ if (this.clusterPlugins != null
+ && this.clusterPlugins.length > 0
+ && !ClusterPluginsSourceConfigurator.resolveClassName()
Review Comment:
Can this be done later in CoreContainer, and rely on a method on the
ClusterPluginsSource interface rather than a comparison of the class name?
##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -647,6 +649,68 @@ private static PluginInfo[]
getBackupRepositoryPluginInfos(List<ConfigNode> cfg)
return configs;
}
+ private static PluginInfo[] getClusterPlugins(SolrResourceLoader loader,
ConfigNode root) {
+ List<PluginInfo> clusterPlugins = new ArrayList<>();
+
+ Collections.addAll(
+ clusterPlugins, getClusterSingletonPluginInfos(loader,
root.getAll("clusterSingleton")));
+
+ PluginInfo replicaPlacementFactory =
getPluginInfo(root.get("replicaPlacementFactory"));
+ if (replicaPlacementFactory != null) {
+ if (replicaPlacementFactory.name != null
+ &&
!replicaPlacementFactory.name.equals(PlacementPluginFactory.PLUGIN_NAME)) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "The replicaPlacementFactory name attribute must be "
+ + PlacementPluginFactory.PLUGIN_NAME);
+ }
+ clusterPlugins.add(replicaPlacementFactory);
+ }
+
+ return clusterPlugins.toArray(new PluginInfo[0]);
+ }
+
+ private static PluginInfo[] getClusterSingletonPluginInfos(
+ SolrResourceLoader loader, List<ConfigNode> nodes) {
+ if (nodes == null || nodes.isEmpty()) {
+ return new PluginInfo[0];
+ }
+
+ List<PluginInfo> plugins =
+ nodes.stream()
+ .map(n -> new PluginInfo(n, n.name(), true, true))
+ .collect(Collectors.toList());
+
+ // Cluster plugin names must be unique
Review Comment:
Aren't plugin names supposed to be unique globally, instead of per plugin
type? If so, this should be moved outside of this method specific to
ClusterSingleton. We should also verify for reserved names, i.e., that a name
does not start with a dot.
##########
solr/core/src/java/org/apache/solr/api/NodeConfigClusterPluginsSource.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.api;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Function;
+import org.apache.solr.cluster.placement.PlacementPluginFactory;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.NodeConfig;
+import org.apache.solr.core.PluginInfo;
+import org.apache.solr.handler.admin.ContainerPluginsApi;
+
+/**
+ * Plugin configurations that are defined in solr.xml. This supports immutable
deployments, and the
+ * /cluster/plugin Edit APIs are not available
+ */
+public class NodeConfigClusterPluginsSource implements ClusterPluginsSource {
+
+ private final Map<String, Object> plugins;
+
+ private final ContainerPluginsApi api;
+
+ public NodeConfigClusterPluginsSource(final CoreContainer cc) {
+ api = new ContainerPluginsApi(cc, this);
+ plugins = Map.copyOf(readPlugins(cc.getNodeConfig()));
+ }
+
+ @Override
+ public ContainerPluginsApi.Read getReadApi() {
+ return api.readAPI;
+ }
+
+ @Override
+ public ContainerPluginsApi.Edit getEditApi() {
+ return null;
+ }
+
+ @Override
+ public Map<String, Object> plugins() throws IOException {
+ return plugins;
+ }
+
+ /**
+ * This method should never be invoked because the Edit Apis are not made
available by the plugin
+ *
+ * @throws UnsupportedOperationException always
+ */
+ @Override
+ public void persistPlugins(Function<Map<String, Object>, Map<String,
Object>> modifier) {
+ throw new UnsupportedOperationException(
+ "The NodeConfigContainerPluginsSource does not support updates to
plugin configurations");
+ }
+
+ private static Map<String, Object> readPlugins(final NodeConfig cfg) {
+ Map<String, Object> pluginInfos = new HashMap<>();
+ PluginInfo[] clusterPlugins = cfg.getClusterPlugins();
+ if (clusterPlugins != null) {
+ for (PluginInfo p : clusterPlugins) {
+ Map<String, Object> pluginMap = new HashMap<>();
+ final String pluginName = getPluginName(p);
+ pluginMap.put("name", pluginName);
+ pluginMap.put("class", p.className);
+
+ if (p.initArgs.size() > 0) {
+ Map<String, Object> config = p.initArgs.toMap(new HashMap<>());
+ pluginMap.put("config", config);
+ }
+
+ pluginInfos.put(pluginName, pluginMap);
+ }
+ }
+ return pluginInfos;
+ }
+
+ /**
+ * Get the correct name for a plugin. Custom plugins must have a name set
already, but built-in
+ * plugins may omit the name in solr.xml and require inference here
+ */
+ private static String getPluginName(final PluginInfo pluginInfo) {
+
+ if (pluginInfo.name == null) {
+ if (pluginInfo.type.equals("replicaPlacementFactory")) {
+ return PlacementPluginFactory.PLUGIN_NAME;
+ }
+ }
+
+ return pluginInfo.name;
Review Comment:
This can return null, and likely generate an NPE later on (?).
##########
solr/core/src/test/org/apache/solr/cluster/placement/impl/PlacementPluginIntegrationTest.java:
##########
@@ -124,25 +124,6 @@ public void testConfigurationInSystemProps() {
cc.shutdown();
}
- @Test
- public void testConfigurationInSolrXml() {
Review Comment:
Has this test be moved elsewhere? It is the only end-to-end (well, kindof)
verification that the placement plugin configured in solr.xml is indeed taking
into account in CoreContainer.
--
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
