Skip to content

Commit 8095046

Browse files
committed
Add fail-safe to VirtualShardRoutingHelper and valid configuration tracking test
Signed-off-by: Atri Sharma <atri.jiit@gmail.com>
1 parent 2144e22 commit 8095046

File tree

2 files changed

+36
-1
lines changed

2 files changed

+36
-1
lines changed

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

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,16 @@ public static int resolvePhysicalShardId(IndexMetadata indexMetadata, int vShard
5252
}
5353
}
5454

55-
int virtualShardsPerPhysical = indexMetadata.getNumberOfVirtualShards() / indexMetadata.getNumberOfShards();
55+
int numVirtualShards = indexMetadata.getNumberOfVirtualShards();
56+
int numPhysicalShards = indexMetadata.getNumberOfShards();
57+
58+
if (numVirtualShards < numPhysicalShards || numVirtualShards % numPhysicalShards != 0) {
59+
throw new IllegalArgumentException(
60+
"Virtual shards must be enabled and be a multiple of the number of physical shards to resolve routing."
61+
);
62+
}
63+
64+
int virtualShardsPerPhysical = numVirtualShards / numPhysicalShards;
5665
return vShardId / virtualShardsPerPhysical;
5766
}
5867
}

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,4 +89,30 @@ public void testInvalidOverridesFallBackToRangeBased() {
8989
// vShard 19 -> 19 / 4 = 4
9090
assertEquals(4, VirtualShardRoutingHelper.resolvePhysicalShardId(metadata, 19));
9191
}
92+
93+
public void testResolvePhysicalShardIdInvalidConfigurations() {
94+
int numPhysicalShards = 5;
95+
96+
// Disabled virtual shards
97+
IndexMetadata metadataDisabled = org.mockito.Mockito.mock(IndexMetadata.class);
98+
org.mockito.Mockito.when(metadataDisabled.getNumberOfVirtualShards()).thenReturn(-1);
99+
org.mockito.Mockito.when(metadataDisabled.getNumberOfShards()).thenReturn(numPhysicalShards);
100+
101+
IllegalArgumentException e1 = expectThrows(
102+
IllegalArgumentException.class,
103+
() -> VirtualShardRoutingHelper.resolvePhysicalShardId(metadataDisabled, 0)
104+
);
105+
assertTrue(e1.getMessage().contains("must be enabled and be a multiple"));
106+
107+
// Invalid multiple
108+
IndexMetadata metadataInvalid = org.mockito.Mockito.mock(IndexMetadata.class);
109+
org.mockito.Mockito.when(metadataInvalid.getNumberOfVirtualShards()).thenReturn(13);
110+
org.mockito.Mockito.when(metadataInvalid.getNumberOfShards()).thenReturn(numPhysicalShards);
111+
112+
IllegalArgumentException e2 = expectThrows(
113+
IllegalArgumentException.class,
114+
() -> VirtualShardRoutingHelper.resolvePhysicalShardId(metadataInvalid, 0)
115+
);
116+
assertTrue(e2.getMessage().contains("must be enabled and be a multiple"));
117+
}
92118
}

0 commit comments

Comments
 (0)