SOLR-8332: Factor HttpShardHandler[Factory]'s url shuffling out into a ReplicaListTransformer class.

(Christine Poerschke, Noble Paul)

solr/CHANGES.txt

@@ -156,6 +156,9 @@ Other Changes
 
 * SOLR-9739: JavabinCodec implements PushWriter interface (noble)
 
+* SOLR-8332: Factor HttpShardHandler[Factory]'s url shuffling out into a ReplicaListTransformer class.
+  (Christine Poerschke, Noble Paul)
+
 ==================  6.3.0 ==================
 
 Consult the LUCENE_CHANGES.txt file for additional, low level, changes in this release.

solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java

@@ -17,6 +17,7 @@
 package org.apache.solr.handler.component;
 import java.lang.invoke.MethodHandles;
 import java.net.ConnectException;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
@@ -116,7 +117,7 @@ public class HttpShardHandler extends ShardHandler {
   private List<String> getURLs(String shard, String preferredHostAddress) {
     List<String> urls = shardToURLs.get(shard);
     if (urls == null) {
-      urls = httpShardHandlerFactory.makeURLList(shard);
+      urls = httpShardHandlerFactory.buildURLList(shard);
       if (preferredHostAddress != null && urls.size() > 1) {
         preferCurrentHostForDistributedReq(preferredHostAddress, urls);
       }
@@ -320,6 +321,8 @@ public class HttpShardHandler extends ShardHandler {
       }
     }
 
+    final ReplicaListTransformer replicaListTransformer = httpShardHandlerFactory.getReplicaListTransformer(req);
+
     if (shards != null) {
       List<String> lst = StrUtils.splitSmart(shards, ",", true);
       rb.shards = lst.toArray(new String[lst.size()]);
@@ -404,7 +407,11 @@ public class HttpShardHandler extends ShardHandler {
 
 
       for (int i=0; i<rb.shards.length; i++) {
-        if (rb.shards[i] == null) {
+        final List<String> shardUrls;
+        if (rb.shards[i] != null) {
+          shardUrls = StrUtils.splitSmart(rb.shards[i], "|", true);
+          replicaListTransformer.transform(shardUrls);
+        } else {
           if (clusterState == null) {
             clusterState =  zkController.getClusterState();
             slices = clusterState.getSlicesMap(cloudDescriptor.getCollectionName());
@@ -421,26 +428,25 @@ public class HttpShardHandler extends ShardHandler {
             // throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "no such shard: " + sliceName);
           }
 
-          Map<String, Replica> sliceShards = slice.getReplicasMap();
+          final Collection<Replica> allSliceReplicas = slice.getReplicasMap().values();
-
+          final List<Replica> eligibleSliceReplicas = new ArrayList<>(allSliceReplicas.size());
-          // For now, recreate the | delimited list of equivalent servers
+          for (Replica replica : allSliceReplicas) {
-          StringBuilder sliceShardsStr = new StringBuilder();
-          boolean first = true;
-          for (Replica replica : sliceShards.values()) {
             if (!clusterState.liveNodesContain(replica.getNodeName())
                 || replica.getState() != Replica.State.ACTIVE) {
               continue;
             }
-            if (first) {
+            eligibleSliceReplicas.add(replica);
-              first = false;
-            } else {
-              sliceShardsStr.append('|');
-            }
-            String url = ZkCoreNodeProps.getCoreUrl(replica);
-            sliceShardsStr.append(url);
           }
 
-          if (sliceShardsStr.length() == 0) {
+          replicaListTransformer.transform(eligibleSliceReplicas);
+
+          shardUrls = new ArrayList<>(eligibleSliceReplicas.size());
+          for (Replica replica : eligibleSliceReplicas) {
+            String url = ZkCoreNodeProps.getCoreUrl(replica);
+            shardUrls.add(url);
+          }
+
+          if (shardUrls.isEmpty()) {
             boolean tolerant = rb.req.getParams().getBool(ShardParams.SHARDS_TOLERANT, false);
             if (!tolerant) {
               // stop the check when there are no replicas available for a shard
@@ -448,9 +454,19 @@ public class HttpShardHandler extends ShardHandler {
                   "no servers hosting shard: " + rb.slices[i]);
             }
           }
-
-          rb.shards[i] = sliceShardsStr.toString();
         }
+        // And now recreate the | delimited list of equivalent servers
+        final StringBuilder sliceShardsStr = new StringBuilder();
+        boolean first = true;
+        for (String shardUrl : shardUrls) {
+          if (first) {
+            first = false;
+          } else {
+            sliceShardsStr.append('|');
+          }
+          sliceShardsStr.append(shardUrl);
+        }
+        rb.shards[i] = sliceShardsStr.toString();
       }
     }
     String shards_rows = params.get(ShardParams.SHARDS_ROWS);

solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java

@@ -31,13 +31,13 @@ import org.apache.solr.common.util.StrUtils;
 import org.apache.solr.common.util.URLUtil;
 import org.apache.solr.core.PluginInfo;
 import org.apache.solr.update.UpdateShardHandlerConfig;
+import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.util.DefaultSolrThreadFactory;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
-import java.util.Collections;
 import java.util.List;
 import java.util.Random;
 import java.util.concurrent.ArrayBlockingQueue;
@@ -84,6 +84,8 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory implements org.
 
   private final Random r = new Random();
 
+  private final ReplicaListTransformer shufflingReplicaListTransformer = new ShufflingReplicaListTransformer(r);
+
   // URL scheme to be used in distributed search.
   static final String INIT_URL_SCHEME = "urlScheme";
 
@@ -227,12 +229,12 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory implements org.
   }
 
   /**
-   * Creates a randomized list of urls for the given shard.
+   * Creates a list of urls for the given shard.
    *
    * @param shard the urls for the shard, separated by '|'
    * @return A list of valid urls (including protocol) that are replicas for the shard
    */
-  public List<String> makeURLList(String shard) {
+  public List<String> buildURLList(String shard) {
     List<String> urls = StrUtils.splitSmart(shard, "|", true);
 
     // convert shard to URL
@@ -240,17 +242,14 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory implements org.
       urls.set(i, buildUrl(urls.get(i)));
     }
 
-    //
-    // Shuffle the list instead of use round-robin by default.
-    // This prevents accidental synchronization where multiple shards could get in sync
-    // and query the same replica at the same time.
-    //
-    if (urls.size() > 1)
-      Collections.shuffle(urls, r);
-
     return urls;
   }
 
+  ReplicaListTransformer getReplicaListTransformer(final SolrQueryRequest req)
+  {
+    return shufflingReplicaListTransformer;
+  }
+
   /**
    * Creates a new completion service for use by a single set of distributed requests.
    */

solr/core/src/java/org/apache/solr/handler/component/ReplicaListTransformer.java

@@ -0,0 +1,35 @@
+/*
+ * 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.handler.component;
+
+import java.util.List;
+
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.params.ShardParams;
+
+interface ReplicaListTransformer {
+
+  /**
+   * Transforms the passed in list of choices. Transformations can include (but are not limited to)
+   * reordering of elements (e.g. via shuffling) and removal of elements (i.e. filtering).
+   *
+   * @param choices - a list of choices to transform, typically the choices are {@link Replica} objects but choices
+   * can also be {@link String} objects such as URLs passed in via the {@link ShardParams#SHARDS} parameter.
+   */
+  public void transform(List<?> choices);
+
+}

solr/core/src/java/org/apache/solr/handler/component/ShufflingReplicaListTransformer.java

@@ -0,0 +1,39 @@
+/*
+ * 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.handler.component;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Random;
+
+class ShufflingReplicaListTransformer implements ReplicaListTransformer {
+
+  private final Random r;
+
+  public ShufflingReplicaListTransformer(Random r)
+  {
+    this.r = r;
+  }
+
+  public void transform(List<?> choices)
+  {
+    if (choices.size() > 1) {
+      Collections.shuffle(choices, r);
+    }
+  }
+
+}

solr/core/src/test/org/apache/solr/handler/component/ReplicaListTransformerTest.java

@@ -0,0 +1,163 @@
+/*
+ * 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.handler.component;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.lucene.util.LuceneTestCase;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.params.ModifiableSolrParams;
+import org.apache.solr.common.params.SolrParams;
+import org.apache.solr.request.LocalSolrQueryRequest;
+import org.apache.solr.request.SolrQueryRequest;
+import org.junit.Test;
+
+public class ReplicaListTransformerTest extends LuceneTestCase {
+
+  // A transformer that keeps only matching choices
+  private static class ToyMatchingReplicaListTransformer implements ReplicaListTransformer {
+
+    private final String regex;
+
+    public ToyMatchingReplicaListTransformer(String regex)
+    {
+      this.regex = regex;
+    }
+
+    public void transform(List<?> choices)
+    {
+      Iterator<?> it = choices.iterator();
+      while (it.hasNext()) {
+        Object choice = it.next();
+        final String url;
+        if (choice instanceof String) {
+          url = (String)choice;
+        }
+        else if (choice instanceof Replica) {
+          url = ((Replica)choice).getCoreUrl();
+        } else {
+          url = null;
+        }
+        if (url == null || !url.matches(regex)) {
+          it.remove();
+        }
+      }
+    }
+
+  }
+
+  // A transformer that makes no transformation
+  private static class ToyNoOpReplicaListTransformer implements ReplicaListTransformer {
+
+    public ToyNoOpReplicaListTransformer()
+    {
+    }
+
+    public void transform(List<?> choices)
+    {
+      // no-op
+    }
+
+  }
+
+  @Test
+  public void testTransform() throws Exception {
+
+    final String regex = ".*" + random().nextInt(10) + ".*";
+
+    final ReplicaListTransformer transformer;
+    if (random().nextBoolean()) {
+
+      transformer = new ToyMatchingReplicaListTransformer(regex);
+
+    } else {
+
+      transformer = new HttpShardHandlerFactory() {
+
+        @Override
+        ReplicaListTransformer getReplicaListTransformer(final SolrQueryRequest req)
+        {
+          final SolrParams params = req.getParams();
+
+          if (params.getBool("toyNoTransform", false)) {
+            return new ToyNoOpReplicaListTransformer();
+          }
+
+          final String regex = params.get("toyRegEx");
+          if (regex != null) {
+            return new ToyMatchingReplicaListTransformer(regex);
+          }
+
+          return super.getReplicaListTransformer(req);
+        }
+
+      }.getReplicaListTransformer(
+          new LocalSolrQueryRequest(null,
+              new ModifiableSolrParams().add("toyRegEx", regex)));
+    }
+
+    final List<Replica> inputs = new ArrayList<>();
+    final List<Replica> expectedTransformed = new ArrayList<>();
+
+    final List<String> urls = createRandomUrls();
+    for (int ii=0; ii<urls.size(); ++ii) {
+
+      final String name = "replica"+(ii+1);
+      final String url = urls.get(ii);
+      final Map<String,Object> propMap = new HashMap<String,Object>();
+      propMap.put("base_url", url);
+      // a skeleton replica, good enough for this test's purposes
+      final Replica replica = new Replica(name, propMap);
+
+      inputs.add(replica);
+      if (url.matches(regex)) {
+        expectedTransformed.add(replica);
+      }
+    }
+
+    final List<Replica> actualTransformed = new ArrayList<>(inputs);
+    transformer.transform(actualTransformed);
+
+    assertEquals(expectedTransformed.size(), actualTransformed.size());
+    for (int ii=0; ii<expectedTransformed.size(); ++ii) {
+      assertEquals("mismatch for ii="+ii, expectedTransformed.get(ii), actualTransformed.get(ii));
+    }
+  }
+
+  private final List<String> createRandomUrls() throws Exception {
+    final List<String> urls = new ArrayList<>();
+    maybeAddUrl(urls, "a"+random().nextDouble());
+    maybeAddUrl(urls, "bb"+random().nextFloat());
+    maybeAddUrl(urls, "ccc"+random().nextGaussian());
+    maybeAddUrl(urls, "dddd"+random().nextInt());
+    maybeAddUrl(urls, "eeeee"+random().nextLong());
+    Collections.shuffle(urls, random());
+    return urls;
+  }
+
+  private final void maybeAddUrl(final List<String> urls, final String url) {
+    if (random().nextBoolean()) {
+      urls.add(url);
+    }
+  }
+
+}

solr/core/src/test/org/apache/solr/handler/component/ShufflingReplicaListTransformerTest.java

@@ -0,0 +1,76 @@
+/*
+ * 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.handler.component;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.lucene.util.LuceneTestCase;
+import org.apache.solr.common.cloud.Replica;
+import org.junit.Test;
+
+public class ShufflingReplicaListTransformerTest extends LuceneTestCase {
+
+  private final ShufflingReplicaListTransformer transformer = new ShufflingReplicaListTransformer(random());
+
+  @Test
+  public void testTransformReplicas() throws Exception {
+    final List<Replica> replicas = new ArrayList<>();
+    for (final String url : createRandomUrls()) {
+      replicas.add(new Replica(url, new HashMap<String,Object>()));
+    }
+    implTestTransform(replicas);
+  }
+
+  @Test
+  public void testTransformUrls() throws Exception {
+    final List<String> urls = createRandomUrls();
+    implTestTransform(urls);
+  }
+
+  private <TYPE> void implTestTransform(List<TYPE> inputs) throws Exception {
+    final List<TYPE> transformedInputs = new ArrayList<>(inputs);
+    transformer.transform(transformedInputs);
+
+    final Set<TYPE> inputSet = new HashSet<>(inputs);
+    final Set<TYPE> transformedSet = new HashSet<>(transformedInputs);
+
+    assertTrue(inputSet.equals(transformedSet));
+  }
+
+  private final List<String> createRandomUrls() throws Exception {
+    final List<String> urls = new ArrayList<>();
+    maybeAddUrl(urls, "a"+random().nextDouble());
+    maybeAddUrl(urls, "bb"+random().nextFloat());
+    maybeAddUrl(urls, "ccc"+random().nextGaussian());
+    maybeAddUrl(urls, "dddd"+random().nextInt());
+    maybeAddUrl(urls, "eeeee"+random().nextLong());
+    Collections.shuffle(urls, random());
+    return urls;
+  }
+
+  private final void maybeAddUrl(final List<String> urls, final String url) {
+    if (random().nextBoolean()) {
+      urls.add(url);
+    }
+  }
+
+}