SOLR-17837: Omit PULL replicas from preferredLeader prop distribution (#3451)

Co-authored-by: kliang78 <kliang78@bloomberg.net>
Co-authored-by: Houston Putman <houston@apache.org>

Author: 3 people

solr/CHANGES.txt

@@ -265,6 +265,8 @@ Bug Fixes
 
 * SOLR-17863: Fix race condition in SolrCore's fingerprint cache which caused leader election to hang. (Luke Kot-Zaniewski, Matthew Biscocho)
 
+* SOLR-17837: PULL replica nodes could be marked as "preferredLeader" by BALANCESHARDUNIQUE despite never being able to be elected leader (Kevin Liang via Houston Putman)
+
 Dependency Upgrades
 ---------------------
 (No changes)

solr/core/src/java/org/apache/solr/cloud/ExclusiveSliceProperty.java

@@ -151,6 +151,12 @@ private boolean collectCurrentPropStats() {
           }
           continue;
         }
+        // omit replicas that cannot potentially be leader from preferredLeader
+        // candidates
+        if (SliceMutator.PREFERRED_LEADER_PROP.equals(property)
+            && !replica.getType().leaderEligible) {
+          continue;
+        }
         allHosts.add(replica.getNodeName());
         String nodeName = replica.getNodeName();
         if (StrUtils.isNotBlank(replica.getStr(property))) {

solr/core/src/test/org/apache/solr/cloud/api/collections/TestReplicaProperties.java

@@ -36,7 +36,6 @@
 import org.junit.Test;
 
 public class TestReplicaProperties extends ReplicaPropertiesBase {
-
   public static final String COLLECTION_NAME = "testcollection";
 
   public TestReplicaProperties() {
@@ -49,11 +48,11 @@ public TestReplicaProperties() {
   public void test() throws Exception {
 
     try (CloudSolrClient client = createCloudClient(null)) {
-      // Mix up a bunch of different combinations of shards and replicas in order to exercise
-      // boundary cases. shards, replicationFactor
-      int shards = random().nextInt(5) + 2;
-      int replicationFactor = random().nextInt(2) + 2;
-      createCollection(null, COLLECTION_NAME, shards, replicationFactor, client, null, "conf1");
+      int shards = 5;
+      int tlogNodes = 4;
+      int pullNodes = 4;
+      Map<String, Object> replicaProps = createReplicaProps(0, tlogNodes, pullNodes, shards);
+      createCollection(null, COLLECTION_NAME, replicaProps, client, "conf1");
     }
 
     waitForCollection(ZkStateReader.from(cloudClient), COLLECTION_NAME, 2);
@@ -249,6 +248,14 @@ private void verifyLeaderAssignment(CloudSolrClient client, String collectionNam
         for (Replica replica : slice.getReplicas()) {
           boolean isLeader = replica.getBool("leader", false);
           boolean isPreferred = replica.getBool("property.preferredleader", false);
+          if (!replica.getType().leaderEligible && isPreferred) {
+            lastFailMsg =
+                "Replica "
+                    + replica.getName()
+                    + " of type "
+                    + replica.getType()
+                    + ", that cannot be elected leader, should NOT be marked as a preferred leader";
+          }
           if (isLeader != isPreferred) {
             lastFailMsg =
                 "Replica should NOT have preferredLeader != leader. Preferred: "

solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java

@@ -2359,17 +2359,25 @@ public static void waitForNon403or404or503(SolrClient collectionClient, String b
     fail("Could not find the new collection - " + exp.code() + " : " + baseUrl);
   }
 
+  // for bypassing getPullReplicaCount() - idk why setting pulla replica # should be determined at
+  // class level and
+  //                                       can cause weird behavior
+  protected static Map<String, Object> createReplicaProps(
+      int numNrtReplicas, int numTlogReplicas, int numPullReplicas, int numShards) {
+    return Map.of(
+        ZkStateReader.NRT_REPLICAS, numNrtReplicas,
+        ZkStateReader.TLOG_REPLICAS, numTlogReplicas,
+        ZkStateReader.PULL_REPLICAS, numPullReplicas,
+        CollectionHandlingUtils.NUM_SLICES, numShards);
+  }
+
   protected void createCollection(
       String collName, CloudSolrClient client, int replicationFactor, int numShards)
       throws Exception {
     int numNrtReplicas = useTlogReplicas() ? 0 : replicationFactor;
     int numTlogReplicas = useTlogReplicas() ? replicationFactor : 0;
     Map<String, Object> props =
-        Map.of(
-            ZkStateReader.NRT_REPLICAS, numNrtReplicas,
-            ZkStateReader.TLOG_REPLICAS, numTlogReplicas,
-            ZkStateReader.PULL_REPLICAS, getPullReplicaCount(),
-            CollectionHandlingUtils.NUM_SLICES, numShards);
+        createReplicaProps(numNrtReplicas, numTlogReplicas, getPullReplicaCount(), numShards);
     Map<String, List<Integer>> collectionInfos = new HashMap<>();
     createCollection(collectionInfos, collName, props, client);
   }