dsmiley commented on code in PR #2126:
URL: https://github.com/apache/solr/pull/2126#discussion_r1445581541
##########
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##########
@@ -223,17 +221,23 @@ private NodeConfig(
if (this.clusterSingletonPlugins != null
&& this.clusterSingletonPlugins.length > 0
- &&
!ClusterPluginsSourceConfigurator.resolveClassName(this.clusterPluginsSource)
+ && !ClusterPluginsSourceConfigurator.resolveClassName()
.equals(NodeConfigClusterPluginsSource.class.getName())) {
throw new SolrException(
ErrorCode.SERVER_ERROR,
- "clusterSingleton section found in solr.xml but clusterPluginsSource
is not NodeConfigClusterPluginsSource");
+ "clusterSingleton section found in solr.xml but the property "
+ + CONFIG_EDITING_DISABLED_ARG
+ + " is set to false. clusterSingleton plugins may only be
declared in solr.xml with immutable configs.");
}
setupSharedLib();
initModules();
}
+ public static boolean isImmutableConfigSet() {
Review Comment:
My proposal is that this *not* be configSet specific. So simply rename it
to be more general. Also FYI note that upstream there's a new sys-prop/env-var
system and a dev list discussion.
##########
solr/core/src/java/org/apache/solr/core/NodeConfig.java:
##########
@@ -223,17 +221,23 @@ private NodeConfig(
if (this.clusterSingletonPlugins != null
&& this.clusterSingletonPlugins.length > 0
Review Comment:
Do we need fields for clusterSingletonPlugins specifically? Remember we're
soon expanding this to other types like replica placement and hopefully more.
Could there be a Map of node/cluster/container plugins instead, in other words?
This way you're not singling out one specific type here in this check.
##########
solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java:
##########
@@ -104,9 +105,6 @@
public class SolrConfigHandler extends RequestHandlerBase
implements SolrCoreAware, PermissionNameProvider {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- public static final String CONFIGSET_EDITING_DISABLED_ARG =
"disable.configEdit";
Review Comment:
@janhoy if you didn't notice, we're elevating this very internal
undocumented sys prop to node level to increase the scope beyond config editing
of a core live to also include disabling cluster config edit. Obviously the
name of this sys prop is poor and your new efforts to standardize suggest we
should take the opportunity to have a nicer name?
--
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]
