You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/10/21 07:31:14 UTC
[GitHub] [solr] murblanc commented on a diff in pull request #1089: SOLR-16475 Make the default replica placement plugin configurable
murblanc commented on code in PR #1089:
URL: https://github.com/apache/solr/pull/1089#discussion_r1001442865
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
Review Comment:
Why not pass the fully qualified classname in the system property so that Assign.java does not have to be touched to use a new placement plugin? Security concerns?
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
+ if (defaultPluginId != null) {
+ switch (defaultPluginId.toLowerCase(Locale.ROOT)) {
+ case "simple":
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ break;
+ case "affinity":
+ placementPlugin = (new AffinityPlacementFactory()).createPluginInstance();
+ break;
+ case "minimizecores":
+ placementPlugin = (new MinimizeCoresPlacementFactory()).createPluginInstance();
+ break;
+ case "random":
+ placementPlugin = (new RandomPlacementFactory()).createPluginInstance();
+ break;
+ default:
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid value for system property '"
+ + DEFAULT_PLACEMENT_PLUGIN_SYSPROP
+ + "'. Supported values are 'simple', 'random', 'affinity' and 'minimizecores'");
+ }
+ log.info(
+ "Default replica placement plugin set in {} to {}",
+ DEFAULT_PLACEMENT_PLUGIN_SYSPROP,
+ defaultPluginId);
+ } else {
+ // TODO: Replace this with a better options, such as the AffinityPlacementFactory
Review Comment:
This comment now goes away since you implement a better option.
##########
solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java:
##########
@@ -562,8 +566,36 @@ public static AssignStrategy createAssignStrategy(CoreContainer coreContainer) {
coreContainer.getPlacementPluginFactory().createPluginInstance();
if (placementPlugin == null) {
// Otherwise use the default
- // TODO: Replace this with a better options, such as the AffinityPlacementFactory
- placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ String defaultPluginId = System.getProperty(DEFAULT_PLACEMENT_PLUGIN_SYSPROP);
+ if (defaultPluginId != null) {
+ switch (defaultPluginId.toLowerCase(Locale.ROOT)) {
+ case "simple":
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ break;
+ case "affinity":
+ placementPlugin = (new AffinityPlacementFactory()).createPluginInstance();
+ break;
+ case "minimizecores":
+ placementPlugin = (new MinimizeCoresPlacementFactory()).createPluginInstance();
+ break;
+ case "random":
+ placementPlugin = (new RandomPlacementFactory()).createPluginInstance();
+ break;
+ default:
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR,
+ "Invalid value for system property '"
+ + DEFAULT_PLACEMENT_PLUGIN_SYSPROP
+ + "'. Supported values are 'simple', 'random', 'affinity' and 'minimizecores'");
+ }
+ log.info(
+ "Default replica placement plugin set in {} to {}",
+ DEFAULT_PLACEMENT_PLUGIN_SYSPROP,
+ defaultPluginId);
+ } else {
+ // TODO: Replace this with a better options, such as the AffinityPlacementFactory
+ placementPlugin = (new SimplePlacementFactory()).createPluginInstance();
+ }
}
return new PlacementPluginAssignStrategy(placementPlugin);
Review Comment:
I'm afraid things might get a bit confusing when multiple configurations collide.
There's the default default plugin (Simple), then there's an override to the default (introduced in this PR) and then there's a ZK configured plugin.
It likely makes no practical sense to both have a ZK configured plugin and a default override at the same time. Should we warn here if that's the case?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org