HoustonPutman commented on code in PR #1577:
URL: https://github.com/apache/solr/pull/1577#discussion_r1177962847
##########
solr/solr-ref-guide/modules/configuration-guide/pages/replica-placement-plugins.adoc:
##########
@@ -184,6 +192,24 @@ This property defines an additional constraint that
collections (keys) must be l
Nodes are labeled using the `node_type` system property with the value being
an arbitrary comma-separated list of labels.
Correspondingly, the plugin configuration can specify that a particular
collection must be placed only on the nodes that match at least one of the
(comma-separated) labels defined here.
+`spreadAcrossDomains`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: `false`
+|===
++
+Spread replicas for a given shard/replica type across spread domains, that are
defined by nodes using the system property `spread_domain`
+
+`maxReplicasPerShardInDomain`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: `-1`
+|===
++
+Set a maximum number of replicas (for a given shard and replica type) that can
be placed in a particular spread domain. Fail the request if this can't be
satisfied. This can be used only in combination of `spreadAcrossDomains`.
Review Comment:
```suggestion
Set a maximum number of replicas (for a given shard and replica type) that
can be placed in a particular spread domain. Fail the request if this can't be
satisfied. This can be used only in combination with `spreadAcrossDomains`.
```
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java:
##########
@@ -503,15 +533,203 @@ private String getNodeAZ(Node n, final AttributeValues
attrValues) {
*/
private static class AzWithNodes {
final String azName;
- List<Node> availableNodesForPlacement;
- boolean hasBeenSorted;
-
- AzWithNodes(String azName, List<Node> availableNodesForPlacement) {
+ private final boolean useSpreadDomains;
+ private boolean listIsSorted = false;
+ private final Comparator<Node> nodeComparator;
+ private final Random random;
+ private final List<Node> availableNodesForPlacement;
+ private final AttributeValues attributeValues;
+ private final int maxReplicasCountInDomain;
+ private TreeSet<SpreadDomainWithNodes> sortedSpreadDomains;
+ private final Map<String, Integer> currentSpreadDomainUsageUsage;
+ private int numNodesForPlacement;
+
+ AzWithNodes(
+ String azName,
+ List<Node> availableNodesForPlacement,
+ boolean useSpreadDomains,
+ Comparator<Node> nodeComparator,
+ Random random,
+ AttributeValues attributeValues,
+ Map<String, Integer> currentSpreadDomainUsageUsage,
+ int maxReplicasCountInDomain) {
this.azName = azName;
this.availableNodesForPlacement = availableNodesForPlacement;
- // Once the list is sorted to an order we're happy with, this flag is
set to true to avoid
- // sorting multiple times unnecessarily.
- this.hasBeenSorted = false;
+ this.useSpreadDomains = useSpreadDomains;
+ this.nodeComparator = nodeComparator;
+ this.random = random;
+ this.attributeValues = attributeValues;
+ this.currentSpreadDomainUsageUsage = currentSpreadDomainUsageUsage;
+ this.numNodesForPlacement = availableNodesForPlacement.size();
+ this.maxReplicasCountInDomain =
+ maxReplicasCountInDomain > 0 ? maxReplicasCountInDomain :
Integer.MAX_VALUE;
+ }
+
+ private boolean hasBeenSorted() {
+ return (useSpreadDomains && sortedSpreadDomains != null)
+ || (!useSpreadDomains && listIsSorted);
+ }
+
+ void ensureSorted() {
+ if (!hasBeenSorted()) {
+ sort();
+ }
+ }
+
+ private void sort() {
+ assert !listIsSorted && sortedSpreadDomains == null
+ : "We shouldn't be sorting this list again";
+
+ // Make sure we do not tend to use always the same nodes (within an
AZ) if all
+ // conditions are identical (well, this likely is not the case since
after having added
+ // a replica to a node its number of cores increases for the next
placement decision,
+ // but let's be defensive here, given that multiple concurrent
placement decisions might
+ // see the same initial cluster state, and we want placement to be
reasonable even in
+ // that case without creating an unnecessary imbalance). For example,
if all nodes have
+ // 0 cores and same amount of free disk space, ideally we want to pick
a random node for
+ // placement, not always the same one due to some internal ordering.
+ Collections.shuffle(availableNodesForPlacement, random);
+
+ if (useSpreadDomains) {
+ // When we use spread domains, we don't just sort the list of nodes,
instead we generate a
+ // TreeSet of SpreadDomainWithNodes,
+ // sorted by the number of times the domain has been used. Each
+ // SpreadDomainWithNodes internally contains the list of nodes that
belong to that
+ // particular domain,
+ // and it's sorted internally by the comparator passed to this
+ // class (which is the same that's used when not using spread
domains).
+ // Whenever a node from a particular SpreadDomainWithNodes is
selected as the best
+ // candidate, the call to "removeBestNode" will:
+ // 1. Remove the SpreadDomainWithNodes instance from the TreeSet
+ // 2. Remove the best node from the list within the
SpreadDomainWithNodes
+ // 3. Increment the count of times the domain has been used
+ // 4. Re-add the SpreadDomainWithNodes instance to the TreeSet if
there are still nodes
+ // available
+ HashMap<String, List<Node>> spreadDomainToListOfNodesMap = new
HashMap<>();
+ for (Node node : availableNodesForPlacement) {
+ spreadDomainToListOfNodesMap
+ .computeIfAbsent(
+ attributeValues
+ .getSystemProperty(node,
AffinityPlacementConfig.SPREAD_DOMAIN_SYSPROP)
+ .get(),
+ k -> new ArrayList<>())
+ .add(node);
+ }
+ sortedSpreadDomains =
+ new TreeSet<>(new
SpreadDomainComparator(currentSpreadDomainUsageUsage));
+
+ int i = 0;
+ for (Map.Entry<String, List<Node>> entry :
spreadDomainToListOfNodesMap.entrySet()) {
+ if (maxReplicasCountInDomain != Integer.MAX_VALUE
+ &&
this.currentSpreadDomainUsageUsage.getOrDefault(entry.getKey(), 0)
Review Comment:
I don't think we need the maxReplicasCountInDomain check, since the second
clause will return the same either way. Right?
```suggestion
if
(this.currentSpreadDomainUsageUsage.getOrDefault(entry.getKey(), 0)
```
--
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
