SOLR-17198: AttributeFetcher no longer fails when it observes multiple shard leaders (#2335)

AffinityPlacementFactory can fail if Shard leadership changes occur while it is collecting metrics.


Co-authored-by: Paul McArthur <[email protected]>

Author: pjmcarthur

solr/CHANGES.txt

@@ -27,6 +27,7 @@ Optimizations
 
 Bug Fixes
 ---------------------
+(No changes)
 
 Deprecation Removals
 ----------------------
@@ -139,6 +140,9 @@ Bug Fixes
 
 * SOLR-17197: Fix getting fieldType by its name in FileBasedSpellChecker (Andrey Bozhko via Eric Pugh)
 
+* SOLR-17198: AffinityPlacementFactory can fail if Shard leadership changes occur while it is collecting metrics.
+  (Paul McArthur)
+
 Dependency Upgrades
 ---------------------
 (No changes)

solr/core/src/java/org/apache/solr/cluster/placement/impl/CollectionMetricsBuilder.java

@@ -16,6 +16,7 @@
  */
 package org.apache.solr.cluster.placement.impl;
 
+import java.lang.invoke.MethodHandles;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
@@ -24,10 +25,14 @@
 import org.apache.solr.cluster.placement.ReplicaMetric;
 import org.apache.solr.cluster.placement.ReplicaMetrics;
 import org.apache.solr.cluster.placement.ShardMetrics;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /** Builder class for constructing instances of {@link CollectionMetrics}. */
 public class CollectionMetricsBuilder {
 
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
   final Map<String, ShardMetricsBuilder> shardMetricsBuilders = new HashMap<>();
 
   public Map<String, ShardMetricsBuilder> getShardMetricsBuilders() {
@@ -78,15 +83,13 @@ public ShardMetrics build() {
             ReplicaMetrics metrics = replicaBuilder.build();
             metricsMap.put(name, metrics);
             if (replicaBuilder.leader) {
-              if (leaderMetricsBuilder == null) {
-                leaderMetricsBuilder = replicaBuilder;
-              } else if (!leaderMetricsBuilder.replicaName.equals(replicaBuilder.replicaName)) {
-                throw new RuntimeException(
-                    "two replicas claim to be the shard leader! existing="
-                        + leaderMetricsBuilder
-                        + " and current "
-                        + replicaBuilder);
+              if (leaderMetricsBuilder != null
+                  && !leaderMetricsBuilder.replicaName.equals(replicaBuilder.replicaName)) {
+                log.warn(
+                    "Multiple replicas claim to be shard leader, selecting the latest candidate ({}) for metrics purposes",
+                    replicaBuilder.replicaName);
               }
+              leaderMetricsBuilder = replicaBuilder;
             }
           });
       final ReplicaMetrics finalLeaderMetrics =

solr/core/src/test/org/apache/solr/cluster/placement/impl/CollectionMetricsBuilderTest.java

@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.placement.impl;
+
+import java.util.Arrays;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.cluster.placement.CollectionMetrics;
+import org.apache.solr.cluster.placement.ReplicaMetric;
+import org.apache.solr.cluster.placement.ReplicaMetrics;
+import org.apache.solr.cluster.placement.ShardMetrics;
+import org.junit.Test;
+
+public class CollectionMetricsBuilderTest extends SolrTestCaseJ4 {
+
+  @Test
+  public void testMultipleShardLeaders() {
+    CollectionMetricsBuilder.ReplicaMetricsBuilder r1 =
+        createReplicaMetricsBuilder(
+            "r1", ReplicaMetricImpl.INDEX_SIZE_GB, 1.5 * MetricImpl.GB, true);
+    CollectionMetricsBuilder.ReplicaMetricsBuilder r2 =
+        createReplicaMetricsBuilder(
+            "r2", ReplicaMetricImpl.INDEX_SIZE_GB, 2.5 * MetricImpl.GB, true);
+
+    CollectionMetrics metrics = collectionMetricsFromShardReplicaBuilders("shard1", r1, r2);
+    ShardMetrics shardMetrics = metrics.getShardMetrics("shard1").get();
+
+    assertTrue("Shard metrics not found", shardMetrics.getLeaderMetrics().isPresent());
+    assertTrue(
+        "No metrics were present for leader replica", shardMetrics.getLeaderMetrics().isPresent());
+    ReplicaMetrics leaderMetrics = shardMetrics.getLeaderMetrics().get();
+
+    // Both replicas claimed to be shard leader, so either metric value is acceptable, and an
+    // exception should not be raised
+    Double indexSize = leaderMetrics.getReplicaMetric(ReplicaMetricImpl.INDEX_SIZE_GB).get();
+    assertTrue(
+        "Metric value " + indexSize + " should have matched one of the replica's values",
+        indexSize.equals(1.5) || indexSize.equals(2.5));
+  }
+
+  @Test
+  public void testNoShardLeader() {
+    CollectionMetricsBuilder.ReplicaMetricsBuilder r1 =
+        createReplicaMetricsBuilder(
+            "r1", ReplicaMetricImpl.INDEX_SIZE_GB, 1.5 * MetricImpl.GB, false);
+    CollectionMetricsBuilder.ReplicaMetricsBuilder r2 =
+        createReplicaMetricsBuilder(
+            "r2", ReplicaMetricImpl.INDEX_SIZE_GB, 2.5 * MetricImpl.GB, false);
+
+    CollectionMetrics metrics = collectionMetricsFromShardReplicaBuilders("shard1", r1, r2);
+    assertTrue("Shard metrics not found", metrics.getShardMetrics("shard1").isPresent());
+    ShardMetrics shardMetrics = metrics.getShardMetrics("shard1").get();
+
+    assertFalse(
+        "No replica was leader, so leader metrics should not be present",
+        shardMetrics.getLeaderMetrics().isPresent());
+  }
+
+  private <T> CollectionMetricsBuilder.ReplicaMetricsBuilder createReplicaMetricsBuilder(
+      final String name, final ReplicaMetric<T> metric, final T value, final boolean leader) {
+    CollectionMetricsBuilder.ReplicaMetricsBuilder replicaMetricsBuilder =
+        new CollectionMetricsBuilder.ReplicaMetricsBuilder(name);
+    replicaMetricsBuilder.addMetric(metric, value);
+    replicaMetricsBuilder.setLeader(leader);
+    return replicaMetricsBuilder;
+  }
+
+  private CollectionMetrics collectionMetricsFromShardReplicaBuilders(
+      String shardName, CollectionMetricsBuilder.ReplicaMetricsBuilder... replicaMetrics) {
+    CollectionMetricsBuilder.ShardMetricsBuilder shardMetricsBuilder =
+        new CollectionMetricsBuilder.ShardMetricsBuilder(shardName);
+    Arrays.stream(replicaMetrics)
+        .forEach(r -> shardMetricsBuilder.replicaMetricsBuilders.put(r.replicaName, r));
+
+    CollectionMetricsBuilder builder = new CollectionMetricsBuilder();
+    builder.shardMetricsBuilders.put(shardName, shardMetricsBuilder);
+
+    return builder.build();
+  }
+}