[GitHub] [solr] dsmiley commented on a diff in pull request #864: SOLR-16194 Don't overwrite list of collections in a Routed Alias when…

[GitBox <gi...@apache.org>]

dsmiley commented on code in PR #864:
URL: https://github.com/apache/solr/pull/864#discussion_r873286055


##########
solr/core/src/java/org/apache/solr/servlet/HttpSolrCall.java:
##########
@@ -416,7 +416,7 @@ protected List<String> resolveCollectionListOrAlias(String collectionStr) {
     }
     List<String> result = null;
     LinkedHashSet<String> uniqueList = null;
-    Aliases aliases = getAliases();
+    Aliases aliases = cores.getAliases();

Review Comment:
   You can remove the old `getAliases` method that you no longer call



##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2299,6 +2300,25 @@ public long getStatus() {
     return status;
   }
 
+  /**
+   * Retrieve the aliases from zookeeper. This is typically cached and does not hit zookeeper after
+   * the first use.
+   *
+   * @return an immutable instance of {@code Aliases} accurate as of at the time this method is
+   *     invoked, less any zookeeper update lag.
+   * @throws RuntimeException if invoked on a {@code CoreContainer} where {@link
+   *     #isZooKeeperAware()} returns false
+   */
+  public Aliases getAliases() {
+    if (isZooKeeperAware()) {
+      return getZkController().getZkStateReader().getAliases();
+    } else {
+      // fail fast because it's programmer error, but give slightly more info than NPE.
+      throw new RuntimeException(

Review Comment:
   nit: I would use IllegalStateException



##########
solr/core/src/test/org/apache/solr/cloud/CreateRoutedAliasTest.java:
##########
@@ -231,7 +217,106 @@ public void testV1() throws Exception {
     assertNotNull(meta);
     assertEquals("evt_dt", meta.get("router.field"));
     assertEquals("_default", meta.get("create-collection.collection.configName"));
-    assertEquals(null, meta.get("start"));
+    assertNull(meta.get("start"));
+  }
+
+  @Test
+  public void testUpdateRoudetedAliasDoesNotChangeCollectionList() throws Exception {
+
+    final String aliasName = getSaferTestName();
+    Instant start = Instant.now().truncatedTo(ChronoUnit.HOURS); // mostly make sure no millis
+    createTRAv1(aliasName, start);
+
+    String initialCollectionName =
+        TimeRoutedAlias.formatCollectionNameFromInstant(aliasName, start);
+    assertCollectionExists(initialCollectionName);
+
+    // Note that this is convenient for the test because it imples a different collection name, but
+    // doing this is an advanced operation, typically preceded by manual collection creations and
+    // manual

Review Comment:
   the comment newline flow is bad.  Maybe use `/*` style; perhaps Spotless reflows that better?



-- 
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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org