[GitHub] [solr] bruno-roustant commented on a diff in pull request #1215: DocRouter: strengthen abstraction

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

bruno-roustant commented on code in PR #1215:
URL: https://github.com/apache/solr/pull/1215#discussion_r1054213209


##########
solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateCmd.java:
##########
@@ -253,7 +252,7 @@ private void migrateKey(
             SHARD_ID_PROP,
             sourceSlice.getName(),
             "routeKey",
-            SolrIndexSplitter.getRouteKey(splitKey) + "!",
+            sourceRouter.getRouteKeyNoSuffix(splitKey) + "!",

Review Comment:
   Above, in the existing code, there is a cast to CompositeIdRouter. It is not clear to me if it is known that the router must be of type CompositeIdRouter. Should we add a check like checkRouterSupportsSplitKey()?



##########
solr/core/src/java/org/apache/solr/update/SolrIndexSplitter.java:
##########
@@ -765,18 +766,11 @@ static FixedBitSet[] split(
     return docSets;
   }
 
-  public static String getRouteKey(String idString) {
-    int idx = idString.indexOf(CompositeIdRouter.SEPARATOR);
-    if (idx <= 0) return null;
-    String part1 = idString.substring(0, idx);
-    int commaIdx = part1.indexOf(CompositeIdRouter.bitsSeparator);
-    if (commaIdx > 0 && commaIdx + 1 < part1.length()) {
-      char ch = part1.charAt(commaIdx + 1);
-      if (ch >= '0' && ch <= '9') {
-        part1 = part1.substring(0, commaIdx);
-      }
+  private static void checkRouterSupportsSplitKey(HashBasedRouter hashRouter, String splitKey) {

Review Comment:
   The expectation is much clearer with this method.
   I'm not familiar with the other DocRouter. Does this mean that split is not supported at all with other DocRouter types?



##########
solr/core/src/java/org/apache/solr/handler/admin/SplitOp.java:
##########
@@ -263,8 +263,9 @@ private void handleGetRanges(CoreAdminHandler.CallInfo it, String coreName) thro
         DocCollection collection = clusterState.getCollection(collectionName);
         String sliceName = parentCore.getCoreDescriptor().getCloudDescriptor().getShardId();
         Slice slice = collection.getSlice(sliceName);
-        DocRouter router =
-            collection.getRouter() != null ? collection.getRouter() : DocRouter.DEFAULT;
+        CompositeIdRouter router =

Review Comment:
   As I understand, the router was 'expected' to be a CompositeIdRouter, even if not clear here, because of the code below manipulating the terms and expecting to find a CompositeIdRouter.SEPARATOR.
   It becomes clearer.
   Should we add a check like checkRouterSupportsSplitKey() with a clearer exception?



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