SOLR-13696: Category and Dimensional routed-alias test suites run again, with a create-alias fix and a time-route fix

changelog/unreleased/SOLR-13696-dimensional-routed-alias.yml

@@ -0,0 +1,7 @@
+title: Creating a dimensional routed alias through the V2 collections API no longer fails for missing router parameters, and time routed alias routing no longer throws a ClassCastException when a document timestamp is still a String
+type: fixed
+authors:
+  - name: Nick Shanin
+links:
+  - name: SOLR-13696
+    url: https://issues.apache.org/jira/browse/SOLR-13696

solr/core/src/java/org/apache/solr/cloud/api/collections/RoutedAlias.java

@@ -44,6 +44,7 @@
 import org.apache.solr.handler.admin.CollectionsHandler;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.update.AddUpdateCommand;
+import org.apache.solr.util.TestInjection;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -433,7 +434,11 @@ private String doPreemptive(
 
   private void preemptiveAsync(Runnable r, SolrCore core) {
     preemptiveCreateOnceAlready = true;
-    core.runAsync(r);
+    core.runAsync(
+        () -> {
+          TestInjection.injectPreemptiveCreateLatch();
+          r.run();
+        });
   }
 
   private SolrException unknownCreateType() {

solr/core/src/java/org/apache/solr/cloud/api/collections/TimeRoutedAlias.java

@@ -218,8 +218,8 @@ public String computeInitialCollectionName() {
   @Override
   String[] formattedRouteValues(SolrInputDocument doc) {
     String routeField = getRouteField();
-    Date fieldValue = (Date) doc.getFieldValue(routeField);
-    String dest = calcCandidateCollection(fieldValue.toInstant()).getDestinationCollection();
+    Instant fieldValue = parseRouteKey(doc.getFieldValue(routeField));
+    String dest = calcCandidateCollection(fieldValue).getDestinationCollection();
     int nonValuePrefix =
         getAliasName().length() + getRoutedAliasType().getSeparatorPrefix().length();
     return new String[] {dest.substring(nonValuePrefix)};

solr/core/src/java/org/apache/solr/handler/admin/api/CreateAlias.java

@@ -20,6 +20,8 @@
 import static org.apache.solr.client.solrj.request.beans.V2ApiConstants.COLLECTIONS;
 import static org.apache.solr.cloud.api.collections.RoutedAlias.CATEGORY;
 import static org.apache.solr.cloud.api.collections.RoutedAlias.CREATE_COLLECTION_PREFIX;
+import static org.apache.solr.cloud.api.collections.RoutedAlias.DIMENSIONAL;
+import static org.apache.solr.cloud.api.collections.RoutedAlias.ROUTER_FIELD;
 import static org.apache.solr.cloud.api.collections.RoutedAlias.ROUTER_TYPE_NAME;
 import static org.apache.solr.cloud.api.collections.RoutedAlias.TIME;
 import static org.apache.solr.cloud.api.collections.TimeRoutedAlias.ROUTER_MAX_FUTURE;
@@ -35,6 +37,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import org.apache.solr.client.api.endpoint.CreateAliasApi;
@@ -123,10 +126,19 @@ public static ZkNodeProps createRemoteMessageForRoutedAlias(CreateAliasRequestBo
     remoteMessage.put(NAME, requestBody.name);
 
     if (requestBody.routers.size() > 1) { // Multi-dimensional alias
+      final List<String> dimensionTypes = new ArrayList<>();
+      final List<String> dimensionFields = new ArrayList<>();
       for (int i = 0; i < requestBody.routers.size(); i++) {
-        createValidationHelper(requestBody.routers.get(i))
-            .addRemoteMessageProperties(remoteMessage, "router." + i + ".");
+        final RoutedAliasProperties router = requestBody.routers.get(i);
+        final RoutedAliasValidationHelper helper = createValidationHelper(router);
+        helper.addRemoteMessageProperties(remoteMessage, "router." + i + ".");
+        dimensionTypes.add(helper.getRouterType().name());
+        dimensionFields.add(router.field);
       }
+      // The overseer also requires the top level router name and field, in the same shape
+      // that SolrJ sends for a dimensional alias.
+      remoteMessage.put(ROUTER_TYPE_NAME, DIMENSIONAL + String.join(",", dimensionTypes) + "]");
+      remoteMessage.put(ROUTER_FIELD, String.join(",", dimensionFields));
     } else if (requestBody.routers.size() == 1) { // Single dimensional alias
       createValidationHelper(requestBody.routers.get(0))
           .addRemoteMessageProperties(remoteMessage, "router.");
@@ -238,6 +250,8 @@ private interface RoutedAliasValidationHelper {
 
     void addRemoteMessageProperties(Map<String, Object> remoteMessage, String prefix);
 
+    RoutedAliasTypes getRouterType();
+
     default void ensureRequiredFieldPresent(Object val, String name) {
       if (val == null) {
         throw new SolrException(
@@ -282,6 +296,11 @@ public void validate() {
       }
     }
 
+    @Override
+    public RoutedAliasTypes getRouterType() {
+      return RoutedAliasTypes.TIME;
+    }
+
     @Override
     public void addRemoteMessageProperties(Map<String, Object> remoteMessage, String prefix) {
       remoteMessage.put(prefix + CoreAdminParams.NAME, TIME);
@@ -327,6 +346,11 @@ public void validate() {
       ensureRequiredFieldPresent(aliasProperties.field, "'field' on category routed alias");
     }
 
+    @Override
+    public RoutedAliasTypes getRouterType() {
+      return RoutedAliasTypes.CATEGORY;
+    }
+
     @Override
     public void addRemoteMessageProperties(Map<String, Object> remoteMessage, String prefix) {
       remoteMessage.put(prefix + CoreAdminParams.NAME, CATEGORY);

solr/core/src/java/org/apache/solr/util/TestInjection.java

@@ -143,6 +143,8 @@ static Random random() { // non-private for testing
 
   public static volatile CountDownLatch reindexLatch = null;
 
+  public static volatile CountDownLatch preemptiveCreateLatch = null;
+
   public static volatile String reindexFailure = null;
 
   public static volatile String failIndexFingerprintRequests = null;
@@ -213,6 +215,7 @@ public static void reset() {
     splitLatch = null;
     directUpdateLatch = null;
     reindexLatch = null;
+    preemptiveCreateLatch = null;
     reindexFailure = null;
     prepRecoveryOpPauseForever = null;
     countPrepRecoveryOpPauseForever = new AtomicInteger(0);
@@ -631,6 +634,17 @@ public static boolean injectReindexLatch() {
     return true;
   }
 
+  public static void injectPreemptiveCreateLatch() {
+    CountDownLatch latch = preemptiveCreateLatch;
+    if (latch != null) {
+      try {
+        latch.await(30, TimeUnit.SECONDS);
+      } catch (InterruptedException e) {
+        Thread.currentThread().interrupt();
+      }
+    }
+  }
+
   private static Pair<Boolean, Integer> parseValue(final String raw) {
     if (raw == null) return new Pair<>(false, 0);
     Matcher m = ENABLED_PERCENT.matcher(raw);

solr/core/src/test/org/apache/solr/handler/admin/api/CreateAliasAPITest.java

@@ -305,8 +305,10 @@ public void testRemoteMessageCreationForMultiDimensionalRoutedAlias() throws Exc
     validateRunCommand(
         CollectionParams.CollectionAction.CREATEALIAS,
         message -> {
-          assertEquals(10, message.size());
+          assertEquals(12, message.size());
           assertEquals("someAliasName", message.get("name"));
+          assertEquals("Dimensional[TIME,CATEGORY]", message.get("router.name"));
+          assertEquals("someField,someField", message.get("router.field"));
           assertEquals("time", message.get("router.0.name"));
           assertEquals("someField", message.get("router.0.field"));
           assertEquals("NOW/HOUR", message.get("router.0.start"));

solr/core/src/test/org/apache/solr/update/processor/CategoryRoutedAliasUpdateProcessorTest.java

@@ -50,7 +50,7 @@ public class CategoryRoutedAliasUpdateProcessorTest extends RoutedAliasUpdatePro
     "Constructor", "Heart of Gold", "Stunt Ship", "B-ark", "Bi$tromath"
   };
 
-  private static final String categoryField = "ship_name_en";
+  private static final String categoryField = "ship_name_s";
   private static final String intField = "integer_i";
 
   private int lastDocId = 0;

solr/core/src/test/org/apache/solr/update/processor/DimensionalRoutedAliasUpdateProcessorTest.java

@@ -23,6 +23,7 @@
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.time.Instant;
+import java.time.temporal.ChronoUnit;
 import java.util.Collections;
 import java.util.Date;
 import java.util.List;
@@ -127,7 +128,7 @@ public void testTimeCat() throws Exception {
         ap("tabby", "calico"));
 
     testFailedDocument("shorthair", "2017-10-23T00:00:00Z", "couldn't be routed");
-    testFailedDocument("shorthair", "2020-10-23T00:00:00Z", "too far in the future");
+    testFailedDocument("shorthair", tooFarInTheFuture(), "too far in the future");
     testFailedDocument(null, "2019-07-02T00:00:00Z", "Route value is null");
     testFailedDocument("foo__CRA__bar", "2019-07-02T00:00:00Z", "7 character sequence __CRA__");
     testFailedDocument("fóóCRAóóbar", "2019-07-02T00:00:00Z", "7 character sequence __CRA__");
@@ -278,18 +279,22 @@ Here we need to be testing that something that should not be created (extra pree
         ap("shorthair", "tabby", "calico"));
 
     // verify that all the documents ended up in the right collections.
+    // [shard] is requested with style='urls' because the replica core URLs
+    // contain the collection name, which encodes the category and the day
+    // each document should have been routed to. The default [shard] style
+    // returns only the shard id, which names neither.
     QueryResponse resp =
         solrClient.query(
             getAlias(),
             params(
                 "q", "*:*",
                 "rows", "100",
-                "fl", "*,[shard]",
+                "fl", "*,shard:[shard style='urls']",
                 "sort", "id asc"));
     SolrDocumentList results = resp.getResults();
     assertEquals(18, results.getNumFound());
     for (SolrDocument result : results) {
-      String shard = String.valueOf(result.getFieldValue("[shard]"));
+      String shard = String.valueOf(result.getFieldValue("shard"));
       String cat = String.valueOf(result.getFieldValue("cat_s"));
       Date date = (Date) result.getFieldValue("timestamp_dt");
       String day = date.toInstant().toString().split("T")[0];
@@ -349,7 +354,7 @@ public void testCatTime() throws Exception {
         ap("tabby", "calico"));
 
     testFailedDocument("shorthair", "2017-10-23T00:00:00Z", "couldn't be routed");
-    testFailedDocument("shorthair", "2020-10-23T00:00:00Z", "too far in the future");
+    testFailedDocument("shorthair", tooFarInTheFuture(), "too far in the future");
     testFailedDocument(null, "2019-07-02T00:00:00Z", "Route value is null");
     testFailedDocument("foo__CRA__bar", "2019-07-02T00:00:00Z", "7 character sequence __CRA__");
     testFailedDocument("fóóCRAóóbar", "2019-07-02T00:00:00Z", "7 character sequence __CRA__");
@@ -502,18 +507,22 @@ Here we need to be testing that something that should not be created (extra pree
         ap("shorthair", "tabby", "calico"));
 
     // verify that all the documents ended up in the right collections.
+    // [shard] is requested with style='urls' because the replica core URLs
+    // contain the collection name, which encodes the category and the day
+    // each document should have been routed to. The default [shard] style
+    // returns only the shard id, which names neither.
     QueryResponse resp =
         solrClient.query(
             getAlias(),
             params(
                 "q", "*:*",
                 "rows", "100",
-                "fl", "*,[shard]",
+                "fl", "*,shard:[shard style='urls']",
                 "sort", "id asc"));
     SolrDocumentList results = resp.getResults();
     assertEquals(18, results.getNumFound());
     for (SolrDocument result : results) {
-      String shard = String.valueOf(result.getFieldValue("[shard]"));
+      String shard = String.valueOf(result.getFieldValue("shard"));
       String cat = String.valueOf(result.getFieldValue("cat_s"));
       Date date = (Date) result.getFieldValue("timestamp_dt");
       String day = date.toInstant().toString().split("T")[0];
@@ -603,6 +612,14 @@ private void assertCatTimeInvariants(String[] expectedCols, String[] categories)
     assertEquals("COLS FOUND:" + cols, expectedCols.length, cols.size());
   }
 
+  // Computed relative to now so the document is always beyond router.maxFutureMs and is
+  // rejected by TimeRoutedAlias.validateRouteValue. A fixed "future" date eventually becomes
+  // the past; the document is then routed instead, and creating the collections between the
+  // alias start and that date one day at a time stalls the update until it times out.
+  private static String tooFarInTheFuture() {
+    return Instant.now().plus(30, ChronoUnit.DAYS).toString();
+  }
+
   private void testFailedDocument(String category, String timestamp, String errorMsg)
       throws SolrServerException, IOException {
     try {

solr/core/src/test/org/apache/solr/update/processor/RoutedAliasUpdateProcessorTest.java

@@ -31,8 +31,6 @@
 import java.util.TreeSet;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
-import java.util.stream.Collectors;
-import org.apache.lucene.tests.util.LuceneTestCase.AwaitsFix;
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.impl.CloudSolrClient;
@@ -60,17 +58,29 @@
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.response.SolrQueryResponse;
 import org.apache.solr.update.UpdateCommand;
+import org.junit.After;
 import org.junit.Ignore;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-@AwaitsFix(bugUrl = "https://issues.apache.org/jira/browse/SOLR-13696")
 @Ignore // don't try to run abstract base class
 public abstract class RoutedAliasUpdateProcessorTest extends SolrCloudTestCase {
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
   private static final String intField = "integer_i";
 
+  /**
+   * The subclasses create a fresh cluster in every test method (their {@code @Before} calls {@code
+   * configureCluster}), because each class routes to a fixed alias name whose collections cannot be
+   * shared between methods. The framework only shuts down the cluster still referenced when the
+   * class ends, so each method's cluster is shut down here; otherwise the earlier clusters stay
+   * open and their cores, clients and directories fail the suite's resource tracking checks.
+   */
+  @After
+  public void shutdownMethodCluster() throws Exception {
+    shutdownCluster();
+  }
+
   void waitColAndAlias(String alias, String separator, final String suffix, int slices)
       throws InterruptedException {
     // collection to exist
@@ -317,8 +327,6 @@ void addDocsAndCommit(boolean aliasOnly, SolrInputDocument... solrInputDocuments
               .get(getAlias()));
     }
 
-    int commitWithin = random().nextBoolean() ? -1 : 500; // if -1, we commit explicitly instead
-
     if (random().nextBoolean()) {
       // Send in separate threads. Choose random collection & solrClient
       ExecutorService exec = null;
@@ -330,7 +338,7 @@ void addDocsAndCommit(boolean aliasOnly, SolrInputDocument... solrInputDocuments
         List<Future<UpdateResponse>> futures = new ArrayList<>(solrInputDocuments.length);
         for (SolrInputDocument solrInputDocument : solrInputDocuments) {
           String col = collections.get(random().nextInt(collections.size()));
-          futures.add(exec.submit(() -> solrClient.add(col, solrInputDocument, commitWithin)));
+          futures.add(exec.submit(() -> solrClient.add(col, solrInputDocument)));
         }
         for (Future<UpdateResponse> future : futures) {
           assertUpdateResponse(future.get());
@@ -347,33 +355,18 @@ void addDocsAndCommit(boolean aliasOnly, SolrInputDocument... solrInputDocuments
       // send in a batch.
       String col = collections.get(random().nextInt(collections.size()));
       CloudSolrClient solrClient = cluster.getSolrClient();
-      assertUpdateResponse(solrClient.add(col, Arrays.asList(solrInputDocuments), commitWithin));
+      assertUpdateResponse(solrClient.add(col, Arrays.asList(solrInputDocuments)));
     }
-    String col = collections.get(random().nextInt(collections.size()));
-    if (commitWithin == -1) {
+    // The docs may have been routed to any collection of the alias, so commit each of them.
+    // A commit waits for every replica to open a new searcher. commitWithin does not.
+    List<String> aliasCollections =
+        new CollectionAdminRequest.ListAliases()
+            .process(getSolrClient())
+            .getAliasesAsLists()
+            .get(getAlias());
+    assertNotNull("alias " + getAlias() + " is not listed", aliasCollections);
+    for (String col : aliasCollections) {
       getSolrClient().commit(col);
-    } else {
-      // check that it all got committed eventually
-      String docsQ =
-          "{!terms f=id}"
-              + Arrays.stream(solrInputDocuments)
-                  .map(d -> d.getFieldValue("id").toString())
-                  .collect(Collectors.joining(","));
-      int numDocs = queryNumDocs(docsQ);
-      if (numDocs == solrInputDocuments.length) {
-        System.err.println("Docs committed sooner than expected.  Bug or slow test env?");
-        return;
-      }
-      // wait until it's committed
-      Thread.sleep(commitWithin);
-      // Loop for up to 10 seconds waiting for commit to catch up
-      for (int idx = 0; idx < 100; ++idx) {
-        numDocs = queryNumDocs(docsQ);
-        if (numDocs == solrInputDocuments.length) break;
-        Thread.sleep(100);
-      }
-
-      assertEquals("not committed.  Bug or a slow test?", solrInputDocuments.length, numDocs);
     }
   }
 
@@ -384,11 +377,6 @@ void assertUpdateResponse(UpdateResponse rsp) {
     assertTrue("Expected no errors: " + errors, errors == null || errors.isEmpty());
   }
 
-  private int queryNumDocs(String q) throws SolrServerException, IOException {
-    return (int)
-        getSolrClient().query(getAlias(), params("q", q, "rows", "0")).getResults().getNumFound();
-  }
-
   /** Adds the docs to Solr via {@link #getSolrClient()} with the params */
   @SuppressWarnings("SameParameterValue")
   protected UpdateResponse add(