dsmiley commented on code in PR #2016:
URL: https://github.com/apache/solr/pull/2016#discussion_r1362721051
##########
solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java:
##########
@@ -120,6 +123,33 @@ public void unregisterListener(PluginRegistryListener
listener) {
public ContainerPluginsRegistry(CoreContainer coreContainer, ApiBag apiBag) {
this.coreContainer = coreContainer;
this.containerApiBag = apiBag;
+ loadFromNodeConfig();
+ }
+
+ private void loadFromNodeConfig() {
+ // Load plugins defined in solr.xml. Those plugins are then considered
exactly like
+ // other plugins, e.g., they can be updated or uninstalled.
+ for (PluginInfo pluginInfo :
coreContainer.getNodeConfig().getNodePlugins()) {
+ Map<String, Object> info = new HashMap<>();
+ info.put("name", pluginInfo.name);
+ info.put("class", pluginInfo.className);
+ String version = pluginInfo.attributes.get("version");
+ if (null != version) {
+ info.put("version", version);
+ }
+ String pathPrefix = pluginInfo.attributes.get("path-prefix");
+ if (null != pathPrefix) {
+ info.put("path-prefix", pathPrefix);
+ }
+ if (pluginInfo.initArgs.size() > 0) {
+ info.put("config", pluginInfo.initArgs.toMap(new HashMap<>()));
+ }
+ try {
+ addOrUpdatePlugin(Diff.ADDED, pluginInfo.name, new
PluginMetaHolder(info));
+ } catch (Exception exp) {
+ log.error("Invalid pluginInfo configuration", exp);
Review Comment:
should probably propagate rather than try to continue?
##########
solr/solr-ref-guide/modules/configuration-guide/pages/cluster-plugins.adoc:
##########
@@ -88,24 +88,22 @@ A JSON map of additional plugin configuration parameters.
Plugins that implement `ConfigurablePlugin` interface will be initialized with
a
plugin-specific configuration object deserialized from this map.
-Example plugin configuration:
+Example plugin with configuration:
-[source,bash]
+[source,json]
----
-curl -X POST -H 'Content-type: application/json' -d '{
Review Comment:
Why did you make this change? Seems wrong.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/beans/PluginMeta.java:
##########
@@ -48,6 +48,7 @@ public PluginMeta copy() {
result.name = name;
result.klass = klass;
result.version = version;
+ result.pathPrefix = pathPrefix;
Review Comment:
What effect does this have? If it fixes a bug; deserves another JIRA.
Well; maybe could be super minor; I dunno CC @noblepaul
##########
solr/core/src/test/org/apache/solr/api/ContainerPluginsRegistryTest.java:
##########
@@ -0,0 +1,105 @@
+package org.apache.solr.api;
+
+import java.nio.file.Path;
+import java.util.Map;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cluster.placement.plugins.AffinityPlacementConfig;
+import org.apache.solr.core.CoreContainer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests for {@link ContainerPluginsRegistry} */
+public class ContainerPluginsRegistryTest extends SolrTestCaseJ4 {
Review Comment:
Both standalone and SolrCloud ought to be able to use this functionality --
plugins in solr.xml that you are making possible.
TestContainerPlugin, which I just looked at, is testing the "Cluster Plugin"
API. Would you agree? I wish it had a name reflecting that; although I can
sympathize with its name as the corresponding code is in a class named
"ContainerPluginsApi". But I think we will rename that; maybe in a dedicated
PR after we do the work of this PR. No need to do renames in this PR to strike
"container" out of some internal lingo.
##########
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##########
@@ -154,7 +154,7 @@ private NodeConfig(
MetricsConfig metricsConfig,
Map<String, CacheConfig> cachesConfig,
PluginInfo tracerConfig,
- PluginInfo[] containerPlugins,
+ PluginInfo[] nodePlugins,
Review Comment:
This is on a NodeConfig; all PluginInfo or similar here is a "node plugin"
of some kind in a general sense. Perhaps the field (and accessors) might be
named "otherNodePlugins" or "otherPlugins". It's current name isn't that bad
either TBH.
##########
solr/solr-ref-guide/modules/configuration-guide/pages/cluster-plugins.adoc:
##########
@@ -278,3 +276,34 @@ curl -X POST -H 'Content-type: application/json' -d '{
}'
http://localhost:8983/api/cluster/plugin
----
+
+== Plugin Configuration in solr.xml
+
+Plugins can also be installed by creating `<plugin>` elements in `solr.xml`.
+This might be preferred in some situations, such as enforcing immutable
infrastructure.
+
+TIP: See also the section xref:configuration-guide:configuring-solr-xml.adoc[]
for more information about the `solr.xml` file, where to find it, and how to
edit it.
Review Comment:
Yet it is *that* adoc file that is the canonical reference for solr.xml, so
we must put the meat of the documentation about it there, not here. Cross-link.
##########
solr/core/src/test-files/solr/solr-50-all.xml:
##########
@@ -52,6 +53,10 @@
<str name="zkCredentialsInjector">DefaultZkCredentialsInjector</str>
</solrcloud>
+ <plugin name="testPlugin" class="TestPlugin">
+ <str name="param">value</str>
+ </plugin>
Review Comment:
You are not aware, but I've been trying to get us to stop creating or
modifying test configs, or to at least do so as a last resort if it's really
the best option. So I see this and wonder, what tests uses this file? Is it
just one test that you are augmenting? _Fine_; so be it. If it's multiple, I
want to learn more. Ideally a test writes its config in the test file. I can
show you some good examples of this if asked; there are a sea of tests and it's
hard to know what's good/bad.
##########
solr/core/src/test/org/apache/solr/api/ContainerPluginsRegistryTest.java:
##########
@@ -0,0 +1,105 @@
+package org.apache.solr.api;
+
+import java.nio.file.Path;
+import java.util.Map;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cluster.placement.plugins.AffinityPlacementConfig;
+import org.apache.solr.core.CoreContainer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests for {@link ContainerPluginsRegistry} */
+public class ContainerPluginsRegistryTest extends SolrTestCaseJ4 {
+
+ private CoreContainer cc;
+ private Path solrHome;
+
+ @Before
+ public void beforeTest() {
+ solrHome = createTempDir();
+ }
+
+ @After
+ public void afterTest() {
+ if (null != cc) {
+ cc.shutdown();
+ }
+ cc = null;
+ }
+
+ @Test
+ public void testPluginIsLoadedFromSolrXml() {
+ String solrXml =
+ "<solr><plugin "
+ + "name=\"custom-request-handler\" "
+ +
"class=\"org.apache.solr.api.ContainerPluginsRegistryTest$CustomRequestHandler\""
+ + "/></solr>";
+ cc = createCoreContainer(solrHome, solrXml);
+
+ ContainerPluginsRegistry.ApiInfo apiInfo =
getPlugin("custom-request-handler");
+ assertNull(apiInfo.getInfo().version);
+ assertNull(apiInfo.getInfo().pathPrefix);
+ assertTrue(apiInfo.getInstance() instanceof CustomRequestHandler);
+ }
+
+ @Test
+ public void testPluginIsLoadedFromSolrXmlWithVersion() {
+ String solrXml =
+ "<solr><plugin "
+ + "name=\"custom-request-handler\" "
+ +
"class=\"org.apache.solr.api.ContainerPluginsRegistryTest$CustomRequestHandler\"
"
+ + "version=\"1.0.0\""
+ + "/></solr>";
+ cc = createCoreContainer(solrHome, solrXml);
Review Comment:
glad to see you are testing configs without creating or editing test
solr.xml files :-)
##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -659,6 +660,17 @@ private static PluginInfo[]
getBackupRepositoryPluginInfos(List<ConfigNode> cfg)
return configs;
}
+ private static PluginInfo[] getNodePluginInfos(List<ConfigNode> cfg) {
+ if (cfg.isEmpty()) {
+ return new PluginInfo[0];
+ }
+ PluginInfo[] configs = new PluginInfo[cfg.size()];
+ for (int i = 0; i < cfg.size(); i++) {
+ configs[i] = new PluginInfo(cfg.get(i), "NodePlugin", true, true);
+ }
+ return configs;
+ }
Review Comment:
this is fine, albeit tedious; you may have some fun turning this into a few
lines with Java streams
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
