Fixed review comments and move to range based sharding

Signed-off-by: Atri Sharma <atri.jiit@gmail.com>

Author: atris

server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java

@@ -268,6 +268,14 @@ static Setting<Integer> buildNumberOfShardsSetting() {
     static final String DEFAULT_NUMBER_OF_SHARDS = "opensearch.index.default_number_of_shards";
     static final String MAX_NUMBER_OF_SHARDS = "opensearch.index.max_number_of_shards";
     public static final Setting<Integer> INDEX_NUMBER_OF_SHARDS_SETTING = buildNumberOfShardsSetting();
+    /**
+     * Settings for configuring the number of virtual shards on an index.
+     * <p>
+     * Note: Virtual Shards uses range-based mapping (e.g., {@code vShardId / (V / P)}) to route to
+     * physical shards instead of default modulo hashing. This ensures contiguous bucket mapping
+     * and safe physical shard merging operations later in the lifecycle. The number of virtual shards
+     * must be a multiple of the number of physical shards {@code (V % P == 0)}.
+     */
     public static final Setting<Integer> INDEX_NUMBER_OF_VIRTUAL_SHARDS_SETTING = Setting.intSetting(
         SETTING_NUMBER_OF_VIRTUAL_SHARDS,
         -1,

server/src/main/java/org/opensearch/cluster/metadata/VirtualShardRoutingHelper.java

@@ -10,13 +10,14 @@
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.opensearch.common.annotation.PublicApi;
 
 import java.util.Map;
 
 /**
  * Resolves virtual shard routing to physical shard IDs.
  */
-
+@PublicApi(since = "3.6.0")
 public final class VirtualShardRoutingHelper {
 
     private VirtualShardRoutingHelper() {}
@@ -29,20 +30,27 @@ private VirtualShardRoutingHelper() {}
     public static final String VIRTUAL_SHARDS_CUSTOM_METADATA_KEY = "virtual_shards_routing";
 
     /**
-     * Resolves the Physical Shard ID for a given Virtual Shard ID.
-     *
-     * @param indexMetadata The index metadata.
-     * @param vShardId The virtual shard ID.
-     * @return The physical shard ID.
+     * Resolves the physical shard for a virtual shard id.
      */
     public static int resolvePhysicalShardId(IndexMetadata indexMetadata, int vShardId) {
+        int numVirtualShards = indexMetadata.getNumberOfVirtualShards();
+        int numPhysicalShards = indexMetadata.getNumberOfShards();
+
+        if (numVirtualShards < numPhysicalShards || numVirtualShards % numPhysicalShards != 0) {
+            throw new IllegalArgumentException(
+                "Virtual shards must be enabled and be a multiple of the number of physical shards to resolve routing."
+            );
+        }
+
+        vShardId = Math.floorMod(vShardId, numVirtualShards);
+
         Map<String, String> overrides = indexMetadata.getCustomData(VIRTUAL_SHARDS_CUSTOM_METADATA_KEY);
         if (overrides != null) {
             String pShardIdStr = overrides.get(String.valueOf(vShardId));
             if (pShardIdStr != null) {
                 try {
                     int pShardId = Integer.parseInt(pShardIdStr);
-                    if (pShardId >= 0 && pShardId < indexMetadata.getNumberOfShards()) {
+                    if (pShardId >= 0 && pShardId < numPhysicalShards) {
                         return pShardId;
                     }
                     logger.trace("Invalid override value [{}] for vShard [{}]: out of bounds", pShardId, vShardId);
@@ -52,15 +60,6 @@ public static int resolvePhysicalShardId(IndexMetadata indexMetadata, int vShard
             }
         }
 
-        int numVirtualShards = indexMetadata.getNumberOfVirtualShards();
-        int numPhysicalShards = indexMetadata.getNumberOfShards();
-
-        if (numVirtualShards < numPhysicalShards || numVirtualShards % numPhysicalShards != 0) {
-            throw new IllegalArgumentException(
-                "Virtual shards must be enabled and be a multiple of the number of physical shards to resolve routing."
-            );
-        }
-
         int virtualShardsPerPhysical = numVirtualShards / numPhysicalShards;
         return vShardId / virtualShardsPerPhysical;
     }

server/src/test/java/org/opensearch/cluster/metadata/VirtualShardRoutingHelperTests.java

@@ -39,8 +39,8 @@ public void testResolvePhysicalShardIdDefaultRangeBased() {
     public void testResolvePhysicalShardIdWithOverrides() {
         int numPhysicalShards = 5;
         Map<String, String> overrides = new HashMap<>();
-        overrides.put("7", "1"); // mapped out of standard routing
-        overrides.put("8", "2"); // mapped out of standard routing
+        overrides.put("7", "1");
+        overrides.put("8", "2");
 
         IndexMetadata.Builder builder = IndexMetadata.builder("test")
             .settings(
@@ -57,7 +57,6 @@ public void testResolvePhysicalShardIdWithOverrides() {
         assertEquals(1, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, 7));
         assertEquals(2, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, 8));
 
-        // Default falls back to range-based formula (20 / 5 = 4 vshards per pshard)
         assertEquals(0, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, 0));
         assertEquals(2, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, 9));
     }
@@ -81,19 +80,14 @@ public void testInvalidOverridesFallBackToRangeBased() {
 
         IndexMetadata metadata = builder.build();
 
-        // Standard range-based routing expects 4 vshards per physical shard
-        // vShard 7 -> 7 / 4 = 1
         assertEquals(1, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, 7));
-        // vShard 8 -> 8 / 4 = 2
         assertEquals(2, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, 8));
-        // vShard 19 -> 19 / 4 = 4
         assertEquals(4, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, 19));
     }
 
     public void testResolvePhysicalShardIdInvalidConfigurations() {
         int numPhysicalShards = 5;
 
-        // Disabled virtual shards
         IndexMetadata metadataDisabled = org.mockito.Mockito.mock(IndexMetadata.class);
         org.mockito.Mockito.when(metadataDisabled.getNumberOfVirtualShards()).thenReturn(-1);
         org.mockito.Mockito.when(metadataDisabled.getNumberOfShards()).thenReturn(numPhysicalShards);
@@ -104,7 +98,6 @@ public void testResolvePhysicalShardIdInvalidConfigurations() {
         );
         assertTrue(e1.getMessage().contains("must be enabled and be a multiple"));
 
-        // Invalid multiple
         IndexMetadata metadataInvalid = org.mockito.Mockito.mock(IndexMetadata.class);
         org.mockito.Mockito.when(metadataInvalid.getNumberOfVirtualShards()).thenReturn(13);
         org.mockito.Mockito.when(metadataInvalid.getNumberOfShards()).thenReturn(numPhysicalShards);
@@ -115,4 +108,16 @@ public void testResolvePhysicalShardIdInvalidConfigurations() {
         );
         assertTrue(e2.getMessage().contains("must be enabled and be a multiple"));
     }
+
+    public void testResolvePhysicalShardIdOutOfBoundsNormalization() {
+        int numPhysicalShards = 5;
+        IndexMetadata metadata = org.mockito.Mockito.mock(IndexMetadata.class);
+        org.mockito.Mockito.when(metadata.getNumberOfVirtualShards()).thenReturn(20);
+        org.mockito.Mockito.when(metadata.getNumberOfShards()).thenReturn(numPhysicalShards);
+
+        assertEquals(4, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, -1));
+
+        assertEquals(0, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, 20));
+        assertEquals(1, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, 25));
+    }
 }