You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "HoustonPutman (via GitHub)" <gi...@apache.org> on 2023/04/20 17:10:25 UTC
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1577: SOLR-16719: Let AffinityPlacementFactory have an anti-affinity label
HoustonPutman commented on code in PR #1577:
URL: https://github.com/apache/solr/pull/1577#discussion_r1172807490
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java:
##########
@@ -503,15 +534,199 @@ 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 useAntiAffinity;
+ private boolean listIsSorted = false;
+ private final Comparator<Node> nodeComparator;
+ private final Random random;
+ private final List<Node> availableNodesForPlacement;
+ private final AttributeValues attributeValues;
+ private TreeSet<AffinityGroupWithNodes> sortedAntiAffinityGroups;
+ private final Map<String, Integer> currentAntiAffinityUsage;
+ private int numNodesForPlacement;
+
+ AzWithNodes(
+ String azName,
+ List<Node> availableNodesForPlacement,
+ boolean useAntiAffinity,
+ Comparator<Node> nodeComparator,
+ Random random,
+ AttributeValues attributeValues,
+ Map<String, Integer> currentAntiAffinityUsage) {
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.useAntiAffinity = useAntiAffinity;
+ this.nodeComparator = nodeComparator;
+ this.random = random;
+ this.attributeValues = attributeValues;
+ this.currentAntiAffinityUsage = currentAntiAffinityUsage;
+ this.numNodesForPlacement = availableNodesForPlacement.size();
+ }
+
+ private boolean hasBeenSorted() {
+ return (useAntiAffinity && sortedAntiAffinityGroups != null)
+ || (!useAntiAffinity && listIsSorted);
+ }
+
+ void ensureSorted() {
+ if (!hasBeenSorted()) {
+ sort();
+ }
+ }
+
+ private void sort() {
+ assert !listIsSorted && sortedAntiAffinityGroups == null
Review Comment:
should this be an assert, or should it just return?
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java:
##########
@@ -503,15 +534,199 @@ 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 useAntiAffinity;
+ private boolean listIsSorted = false;
+ private final Comparator<Node> nodeComparator;
+ private final Random random;
+ private final List<Node> availableNodesForPlacement;
+ private final AttributeValues attributeValues;
+ private TreeSet<AffinityGroupWithNodes> sortedAntiAffinityGroups;
+ private final Map<String, Integer> currentAntiAffinityUsage;
+ private int numNodesForPlacement;
+
+ AzWithNodes(
+ String azName,
+ List<Node> availableNodesForPlacement,
+ boolean useAntiAffinity,
+ Comparator<Node> nodeComparator,
+ Random random,
+ AttributeValues attributeValues,
+ Map<String, Integer> currentAntiAffinityUsage) {
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.useAntiAffinity = useAntiAffinity;
+ this.nodeComparator = nodeComparator;
+ this.random = random;
+ this.attributeValues = attributeValues;
+ this.currentAntiAffinityUsage = currentAntiAffinityUsage;
+ this.numNodesForPlacement = availableNodesForPlacement.size();
+ }
+
+ private boolean hasBeenSorted() {
+ return (useAntiAffinity && sortedAntiAffinityGroups != null)
+ || (!useAntiAffinity && listIsSorted);
+ }
+
+ void ensureSorted() {
+ if (!hasBeenSorted()) {
+ sort();
+ }
+ }
+
+ private void sort() {
+ assert !listIsSorted && sortedAntiAffinityGroups == 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 (useAntiAffinity) {
+ // When we use anti-affinity, we don't just sort the list of nodes, instead we generate a
+ // TreeSet of AffinityGroupWithNodes,
+ // sorted by the number of times the affinity label has been used. Each
+ // AffinityGroupWithNodes internally contains the list of nodes that use a particular
+ // affinity
+ // label, and it's sorted internally by the comparator passed to this
+ // class (which is the same that's used when not using anti-affinity).
+ // Whenever a node from a particular AffinityGroupWithNodes is selected as the best
+ // candidate, the call to "removeBestNode" will:
+ // 1. Remove the AffinityGroupWithNodes instance from the TreeSet
+ // 2. Remove the best node from the list within the AffinityGroupWithNodes
+ // 3. Increment the count of times the affinity label has been used
+ // 4. Re-add the AffinityGroupWithNodes instance to the TreeSet if there are still nodes
+ // available
+ HashMap<String, List<Node>> antiAffinityNameToListOfNodesMap = new HashMap<>();
+ for (Node node : availableNodesForPlacement) {
+ antiAffinityNameToListOfNodesMap.compute(
+ attributeValues
+ .getSystemProperty(node, AffinityPlacementConfig.ANTI_AFFINITY_SYSPROP)
+ .get(),
+ (k, v) -> {
+ if (v == null) {
+ v = new ArrayList<>();
+ }
+ v.add(node);
+ return v;
+ });
+ }
+ sortedAntiAffinityGroups =
+ new TreeSet<>(new AntiAffinityComparator(currentAntiAffinityUsage));
+
+ int i = 0;
+ for (Map.Entry<String, List<Node>> entry : antiAffinityNameToListOfNodesMap.entrySet()) {
+ // Sort the nodes within the anti-affinity group by the provided comparator
+ entry.getValue().sort(nodeComparator);
+ sortedAntiAffinityGroups.add(
+ new AffinityGroupWithNodes(entry.getKey(), entry.getValue(), i++, nodeComparator));
+ }
+ } else {
+ availableNodesForPlacement.sort(nodeComparator);
+ listIsSorted = true;
+ }
+ }
+
+ Node getBestNode() {
+ assert hasBeenSorted();
+ if (useAntiAffinity) {
+ return sortedAntiAffinityGroups.first().sortedNodesForPlacement.get(0);
+ } else {
+ return availableNodesForPlacement.get(0);
+ }
+ }
+
+ public Node removeBestNode() {
+ assert hasBeenSorted();
+ this.numNodesForPlacement--;
+ if (useAntiAffinity) {
+ // Since this AffinityGroupWithNodes needs to be re-sorted in the sortedAffinityGroups, we
+ // remove it and then
+ // re-add it, once the best node has been removed.
+ AffinityGroupWithNodes group = sortedAntiAffinityGroups.pollFirst();
+ Node n = group.sortedNodesForPlacement.remove(0);
+ this.currentAntiAffinityUsage.compute(
+ group.affinityGroupName,
+ (k, v) -> {
+ if (v == null) {
+ return 1;
+ }
+ return v + 1;
+ });
Review Comment:
```suggestion
this.currentAntiAffinityUsage.merge(
group.affinityGroupName,
1,
Integer::sum);
```
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java:
##########
@@ -503,15 +534,199 @@ 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 useAntiAffinity;
+ private boolean listIsSorted = false;
+ private final Comparator<Node> nodeComparator;
+ private final Random random;
+ private final List<Node> availableNodesForPlacement;
+ private final AttributeValues attributeValues;
+ private TreeSet<AffinityGroupWithNodes> sortedAntiAffinityGroups;
+ private final Map<String, Integer> currentAntiAffinityUsage;
+ private int numNodesForPlacement;
+
+ AzWithNodes(
+ String azName,
+ List<Node> availableNodesForPlacement,
+ boolean useAntiAffinity,
+ Comparator<Node> nodeComparator,
+ Random random,
+ AttributeValues attributeValues,
+ Map<String, Integer> currentAntiAffinityUsage) {
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.useAntiAffinity = useAntiAffinity;
+ this.nodeComparator = nodeComparator;
+ this.random = random;
+ this.attributeValues = attributeValues;
+ this.currentAntiAffinityUsage = currentAntiAffinityUsage;
+ this.numNodesForPlacement = availableNodesForPlacement.size();
+ }
+
+ private boolean hasBeenSorted() {
+ return (useAntiAffinity && sortedAntiAffinityGroups != null)
+ || (!useAntiAffinity && listIsSorted);
+ }
+
+ void ensureSorted() {
+ if (!hasBeenSorted()) {
+ sort();
+ }
+ }
+
+ private void sort() {
+ assert !listIsSorted && sortedAntiAffinityGroups == 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 (useAntiAffinity) {
+ // When we use anti-affinity, we don't just sort the list of nodes, instead we generate a
+ // TreeSet of AffinityGroupWithNodes,
+ // sorted by the number of times the affinity label has been used. Each
+ // AffinityGroupWithNodes internally contains the list of nodes that use a particular
+ // affinity
+ // label, and it's sorted internally by the comparator passed to this
+ // class (which is the same that's used when not using anti-affinity).
+ // Whenever a node from a particular AffinityGroupWithNodes is selected as the best
+ // candidate, the call to "removeBestNode" will:
+ // 1. Remove the AffinityGroupWithNodes instance from the TreeSet
+ // 2. Remove the best node from the list within the AffinityGroupWithNodes
+ // 3. Increment the count of times the affinity label has been used
+ // 4. Re-add the AffinityGroupWithNodes instance to the TreeSet if there are still nodes
+ // available
+ HashMap<String, List<Node>> antiAffinityNameToListOfNodesMap = new HashMap<>();
+ for (Node node : availableNodesForPlacement) {
+ antiAffinityNameToListOfNodesMap.compute(
+ attributeValues
+ .getSystemProperty(node, AffinityPlacementConfig.ANTI_AFFINITY_SYSPROP)
+ .get(),
+ (k, v) -> {
+ if (v == null) {
+ v = new ArrayList<>();
+ }
+ v.add(node);
+ return v;
+ });
Review Comment:
```suggestion
antiAffinityNameToListOfNodesMap.computeIfAbsent(
attributeValues
.getSystemProperty(node, AffinityPlacementConfig.ANTI_AFFINITY_SYSPROP)
.get(),
ArrayList<>::new).add(node);
```
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java:
##########
@@ -503,15 +534,199 @@ 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 useAntiAffinity;
+ private boolean listIsSorted = false;
+ private final Comparator<Node> nodeComparator;
+ private final Random random;
+ private final List<Node> availableNodesForPlacement;
+ private final AttributeValues attributeValues;
+ private TreeSet<AffinityGroupWithNodes> sortedAntiAffinityGroups;
+ private final Map<String, Integer> currentAntiAffinityUsage;
+ private int numNodesForPlacement;
+
+ AzWithNodes(
+ String azName,
+ List<Node> availableNodesForPlacement,
+ boolean useAntiAffinity,
+ Comparator<Node> nodeComparator,
+ Random random,
+ AttributeValues attributeValues,
+ Map<String, Integer> currentAntiAffinityUsage) {
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.useAntiAffinity = useAntiAffinity;
+ this.nodeComparator = nodeComparator;
+ this.random = random;
+ this.attributeValues = attributeValues;
+ this.currentAntiAffinityUsage = currentAntiAffinityUsage;
+ this.numNodesForPlacement = availableNodesForPlacement.size();
+ }
+
+ private boolean hasBeenSorted() {
+ return (useAntiAffinity && sortedAntiAffinityGroups != null)
+ || (!useAntiAffinity && listIsSorted);
+ }
+
+ void ensureSorted() {
+ if (!hasBeenSorted()) {
+ sort();
+ }
+ }
+
+ private void sort() {
+ assert !listIsSorted && sortedAntiAffinityGroups == 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
Review Comment:
I'm not sure I necessarily agree here. There is a case to be made for both consistency and for ultimate spread. But if all things are equal, I'm not sure it's bad to always choose the same node. That's kind of the idea behind [SOLR-15803](https://issues.apache.org/jira/browse/SOLR-15803).
However it will improve spread when multiple create replica requests are sent at the same time. (or create collection, etc)
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java:
##########
@@ -503,15 +534,199 @@ 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 useAntiAffinity;
+ private boolean listIsSorted = false;
+ private final Comparator<Node> nodeComparator;
+ private final Random random;
+ private final List<Node> availableNodesForPlacement;
+ private final AttributeValues attributeValues;
+ private TreeSet<AffinityGroupWithNodes> sortedAntiAffinityGroups;
+ private final Map<String, Integer> currentAntiAffinityUsage;
+ private int numNodesForPlacement;
+
+ AzWithNodes(
+ String azName,
+ List<Node> availableNodesForPlacement,
+ boolean useAntiAffinity,
+ Comparator<Node> nodeComparator,
+ Random random,
+ AttributeValues attributeValues,
+ Map<String, Integer> currentAntiAffinityUsage) {
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.useAntiAffinity = useAntiAffinity;
+ this.nodeComparator = nodeComparator;
+ this.random = random;
+ this.attributeValues = attributeValues;
+ this.currentAntiAffinityUsage = currentAntiAffinityUsage;
+ this.numNodesForPlacement = availableNodesForPlacement.size();
+ }
+
+ private boolean hasBeenSorted() {
+ return (useAntiAffinity && sortedAntiAffinityGroups != null)
+ || (!useAntiAffinity && listIsSorted);
+ }
+
+ void ensureSorted() {
+ if (!hasBeenSorted()) {
+ sort();
+ }
+ }
+
+ private void sort() {
+ assert !listIsSorted && sortedAntiAffinityGroups == 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 (useAntiAffinity) {
+ // When we use anti-affinity, we don't just sort the list of nodes, instead we generate a
+ // TreeSet of AffinityGroupWithNodes,
+ // sorted by the number of times the affinity label has been used. Each
+ // AffinityGroupWithNodes internally contains the list of nodes that use a particular
+ // affinity
+ // label, and it's sorted internally by the comparator passed to this
+ // class (which is the same that's used when not using anti-affinity).
+ // Whenever a node from a particular AffinityGroupWithNodes is selected as the best
+ // candidate, the call to "removeBestNode" will:
+ // 1. Remove the AffinityGroupWithNodes instance from the TreeSet
+ // 2. Remove the best node from the list within the AffinityGroupWithNodes
+ // 3. Increment the count of times the affinity label has been used
+ // 4. Re-add the AffinityGroupWithNodes instance to the TreeSet if there are still nodes
+ // available
+ HashMap<String, List<Node>> antiAffinityNameToListOfNodesMap = new HashMap<>();
+ for (Node node : availableNodesForPlacement) {
+ antiAffinityNameToListOfNodesMap.compute(
+ attributeValues
+ .getSystemProperty(node, AffinityPlacementConfig.ANTI_AFFINITY_SYSPROP)
+ .get(),
+ (k, v) -> {
+ if (v == null) {
+ v = new ArrayList<>();
+ }
+ v.add(node);
+ return v;
+ });
+ }
+ sortedAntiAffinityGroups =
+ new TreeSet<>(new AntiAffinityComparator(currentAntiAffinityUsage));
+
+ int i = 0;
+ for (Map.Entry<String, List<Node>> entry : antiAffinityNameToListOfNodesMap.entrySet()) {
+ // Sort the nodes within the anti-affinity group by the provided comparator
+ entry.getValue().sort(nodeComparator);
+ sortedAntiAffinityGroups.add(
+ new AffinityGroupWithNodes(entry.getKey(), entry.getValue(), i++, nodeComparator));
+ }
+ } else {
+ availableNodesForPlacement.sort(nodeComparator);
+ listIsSorted = true;
+ }
+ }
+
+ Node getBestNode() {
+ assert hasBeenSorted();
Review Comment:
again, probably best to either call sort() or just do `if (!sorted()) {sort()}`. Is there a reason to believe it wouldn't be sorted already?
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java:
##########
@@ -694,6 +913,19 @@ private void makePlacementDecisions(
// the dereferencing below can't be assumed as the entry will not exist in the map.
azToNumReplicas.put(az, azToNumReplicas.get(az) + 1);
}
+ if (doUseAntiAffinity) {
+ affinityLabelsInUse.compute(
+ attrValues
+ .getSystemProperty(
+ replica.getNode(), AffinityPlacementConfig.ANTI_AFFINITY_SYSPROP)
+ .get(),
+ (k, v) -> {
+ if (v == null) {
+ return 1;
+ }
+ return v + 1;
+ });
Review Comment:
```suggestion
affinityLabelsInUse.merge(
attrValues
.getSystemProperty(
replica.getNode(), AffinityPlacementConfig.ANTI_AFFINITY_SYSPROP)
.get(),
1,
Integer::sum);
```
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java:
##########
@@ -703,15 +935,26 @@ private void makePlacementDecisions(
// consideration how many replicas were per AZ, so we can place (or try to place) replicas on
// AZ's that have fewer replicas
- // Get the candidate nodes per AZ in order to build (further down) a mapping of AZ to
- // placement candidates.
Map<String, List<Node>> nodesPerAz = new HashMap<>();
- for (Node node : candidateNodes) {
- String nodeAz = getNodeAZ(node, attrValues);
- List<Node> nodesForAz = nodesPerAz.computeIfAbsent(nodeAz, k -> new ArrayList<>());
- nodesForAz.add(node);
+ if (availabilityZones.size() == 1) {
+ // If AZs are not being used (all undefined for example) or a single AZ exists, we add all
+ // nodes
+ // to the same entry
+ nodesPerAz.put(availabilityZones.iterator().next(), new ArrayList<>(candidateNodes));
+ } else {
+ // Get the candidate nodes per AZ in order to build (further down) a mapping of AZ to
+ // placement candidates.
+ for (Node node : candidateNodes) {
+ String nodeAz = getNodeAZ(node, attrValues);
+ List<Node> nodesForAz = nodesPerAz.computeIfAbsent(nodeAz, k -> new ArrayList<>());
+ nodesForAz.add(node);
+ }
}
+ Comparator<Node> interGroupNodeComparator =
+ new CoresAndDiskComparator(attrValues, coresOnNodes, prioritizedFreeDiskGB);
+ ;
Review Comment:
```suggestion
```
--
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