HoustonPutman commented on code in PR #1709:
URL: https://github.com/apache/solr/pull/1709#discussion_r1239013909
##########
solr/solr-ref-guide/modules/configuration-guide/pages/replica-placement-plugins.adoc:
##########
@@ -173,6 +174,18 @@ The plugin will assume that the secondary collection
replicas are already in pla
+
See the section <<withCollection constraint>> below.
+`withCollectionShards`::
++
+[%autowidth,frame=none]
+|===
+|Optional |Default: none
+|===
++
+Defines an additional constraint that shards of the primary collections (keys)
must be located on the same nodes as the shards of the secondary collections
(values).
Review Comment:
I think this can use clarification. Does it mean that all shards of
collection A will be co-located with all shards of collection B? Or does it
mean that collection A shard 1 will be co-located with collection B shard 1,
and collection A shard 2 will be co-located with collection B shard 2?
If it's the first, then I think we should rethink how this is implemented.
If it's the second, I think this needs to be spelled out pretty explicitly in
this docs section, and the section below.
(After reading the code, it looks like number 2 is what is actually
happening)
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java:
##########
@@ -555,11 +579,14 @@ public Map<Replica, String>
canRemoveReplicas(Collection<Replica> replicas) {
replica.getShard().getCollection().getName(), k -> new
HashMap<>())
.computeIfAbsent(replica.getShard().getShardName(), k -> new
HashSet<>());
replicasRemovedForShard.add(replica);
-
- if (replicasRemovedForShard.size()
- >= getReplicasForShardOnNode(replica.getShard()).size()) {
- replicaRemovalExceptions.put(
- replica, "co-located with replicas of " +
collocatedCollections);
+ // either if all shards are mandatory, or the current one is
mandatory
+ if (mandatoryShardsOrAll.isEmpty()
+ ||
mandatoryShardsOrAll.contains(replica.getShard().getShardName())) {
+ if (replicasRemovedForShard.size()
+ >= getReplicasForShardOnNode(replica.getShard()).size()) {
+ replicaRemovalExceptions.put(
Review Comment:
This exception should likely mention the shard that is affected.
##########
solr/core/src/java/org/apache/solr/cluster/placement/plugins/AffinityPlacementFactory.java:
##########
@@ -177,14 +184,17 @@ private AffinityPlacementPlugin(
Objects.requireNonNull(collectionNodeTypes, "collectionNodeTypes must
not be null");
this.spreadAcrossDomains = spreadAcrossDomains;
this.withCollections = withCollections;
- if (withCollections.isEmpty()) {
- collocatedWith = Map.of();
- } else {
- collocatedWith = new HashMap<>();
- withCollections.forEach(
- (primary, secondary) ->
- collocatedWith.computeIfAbsent(secondary, s -> new
HashSet<>()).add(primary));
- }
+ this.withCollectionShards = withCollectionShards;
+ Map<String, Set<String>> collocated = new HashMap<>();
+ List.of(this.withCollections, this.withCollectionShards)
Review Comment:
Yeah I think I understand why this is happening, but it could benefit from a
comment.
--
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]
