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: [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]
