[GitHub] [lucene-solr] madrob commented on a change in pull request #2265: SOLR-15119 Add logs and make default splitMethod to be LINK

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

madrob commented on a change in pull request #2265:
URL: https://github.com/apache/lucene-solr/pull/2265#discussion_r566467915



##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
##########
@@ -80,16 +83,33 @@ public void call(ClusterState state, ZkNodeProps message, @SuppressWarnings({"ra
     split(state, message,(NamedList<Object>) results);
   }
 
+  /**

Review comment:
       Can we use HTML tags for the list? Otherwise the formatting will get messed up in editor tooltips and on the javadoc page.

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
##########
@@ -382,6 +407,7 @@ public boolean split(ClusterState clusterState, ZkNodeProps message, NamedList<O
         log.debug("Index on shard: {} split into {} successfully", nodeName, subShardNames.size());
       }
 
+      // 8. apply buffered updates on sub-shards
       t = timings.sub("applyBufferedUpdates");
       // apply buffered updates on sub-shards

Review comment:
       duplicated comment

##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/SplitShardCmd.java
##########
@@ -80,16 +83,33 @@ public void call(ClusterState state, ZkNodeProps message, @SuppressWarnings({"ra
     split(state, message,(NamedList<Object>) results);
   }
 
+  /**
+   * Shard splits start here and make additional requests to the host of the parent shard.
+   *
+   * The sequence of requests is as follows:
+   *  1. Verify that there is enough disk space to create sub-shards.
+   *  2. If splitByPrefix is true, make request to get prefix ranges.
+   *  3. If this split was attempted previously and there are lingering sub-shards, delete them.
+   *  4. Create sub-shards in CONSTRUCTION state.
+   *  5. Add an initial replica to each sub-shard.
+   *  6. Request that parent shard wait for children to become ACTIVE.
+   *  7. Execute split: either LINK or REWRITE.
+   *  8. Apply buffered updates to the sub-shards so they are up-to-date with parent.
+   *  9. Determine node placement for additional replicas (but do not create yet).
+   *  10. If replicationFactor is more than 1, set shard state for sub-shards to RECOVERY; else mark ACTIVE.
+   *  11. Create additional replicas of sub-shards.
+   */
   @SuppressWarnings({"rawtypes"})
   public boolean split(ClusterState clusterState, ZkNodeProps message, NamedList<Object> results) throws Exception {
     final String asyncId = message.getStr(ASYNC);
 
+    // get split method, or default to LINK if unspecified
     boolean waitForFinalState = message.getBool(CommonAdminParams.WAIT_FOR_FINAL_STATE, false);
-    String methodStr = message.getStr(CommonAdminParams.SPLIT_METHOD, SolrIndexSplitter.SplitMethod.REWRITE.toLower());
+    String methodStr = message.getStr(CommonAdminParams.SPLIT_METHOD, SolrIndexSplitter.SplitMethod.LINK.toLower());

Review comment:
       This should be called out in upgrade-notes, I think.

##########
File path: solr/core/src/test/org/apache/solr/cloud/SplitShardTest.java
##########
@@ -106,6 +108,50 @@ public void doTest() throws IOException, SolrServerException {
     }
   }
 
+  /**
+   * Create a collection with 3 shards and split them each with a different splitMethod value.
+   * 1. No override specified. Verify that LINK method is used.
+   * 2. REWRITE method specified. Verify that LINK steps are skipped.
+   * 3. Invalid override specified. Verify that split fails.
+   */
+  @Test
+  public void testSplitMethods() throws Exception {
+    CollectionAdminRequest
+            .createCollection(COLLECTION_NAME, "conf", 3, 1)
+            .process(cluster.getSolrClient());
+
+    cluster.waitForActiveCollection(COLLECTION_NAME, 3, 3);
+
+    CollectionAdminRequest.SplitShard splitShard = CollectionAdminRequest.splitShard(COLLECTION_NAME)
+            .setShardName("shard1");
+    SolrResponse response = splitShard.process(cluster.getSolrClient());
+    assertTrue(response.getResponse().toString().contains("hardLinkCopy"));

Review comment:
       nit: assertThat will get you a better failure message




----------------------------------------------------------------
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.

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



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