Address review comments

Signed-off-by: Sorabh Hamirwasia <[email protected]>

CHANGELOG.md

@@ -7,7 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Added
 - Add server version as REST response header [#6583](https://github.com/opensearch-project/OpenSearch/issues/6583)
 - Start replication checkpointTimers on primary before segments upload to remote store. ([#8221]()https://github.com/opensearch-project/OpenSearch/pull/8221)
-- Introduce new static cluster setting to control slice computation for concurrent segment search. ([#8847](https://github.com/opensearch-project/OpenSearch/pull/8847))
+- Introduce new static cluster setting to control slice computation for concurrent segment search. ([#8847](https://github.com/opensearch-project/OpenSearch/pull/8884))
 
 ### Dependencies
 - Bump `org.apache.logging.log4j:log4j-core` from 2.17.1 to 2.20.0 ([#8307](https://github.com/opensearch-project/OpenSearch/pull/8307))

server/src/main/java/org/opensearch/search/SearchBootstrapSettings.java

@@ -13,6 +13,8 @@
 
 /**
  * Keeps track of all the search related node level settings which can be accessed via static methods
+ *
+ * @opensearch.internal
  */
 public class SearchBootstrapSettings {
     // settings to configure maximum slice created per search request using OS custom slice computation mechanism. Default lucene
@@ -21,6 +23,7 @@ public class SearchBootstrapSettings {
     public static final int CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE = -1;
 
     // value <= 0 means lucene slice computation will be used
+    // this setting will be updated to dynamic setting as part of https://github.com/opensearch-project/OpenSearch/issues/8870
     public static final Setting<Integer> CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING = Setting.intSetting(
         CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY,
         CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE,
@@ -32,7 +35,12 @@ public static void initialize(Settings openSearchSettings) {
         settings = openSearchSettings;
     }
 
-    public static int getValueAsInt(String settingName, int defaultValue) {
-        return (settings != null) ? settings.getAsInt(settingName, defaultValue) : defaultValue;
+    public static int getTargetMaxSlice() {
+        return (settings != null)
+            ? settings.getAsInt(
+                CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY,
+                CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE
+            )
+            : CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE;
     }
 }

server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java

@@ -451,11 +451,9 @@ public CollectionStatistics collectionStatistics(String field) throws IOExceptio
      */
     @Override
     public LeafSlice[] slices(List<LeafReaderContext> leaves) {
-        final int target_max_slices = SearchBootstrapSettings.getValueAsInt(
-            SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY,
-            SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE
-        );
-        return slicesInternal(leaves, target_max_slices);
+        // For now using the static setting to get the targetMaxSlice value. It will be updated to dynamic mechanism as part of
+        // https://github.com/opensearch-project/OpenSearch/issues/8870 when lucene changes are available
+        return slicesInternal(leaves, SearchBootstrapSettings.getTargetMaxSlice());
     }
 
     public DirectoryReader getDirectoryReader() {
@@ -539,15 +537,15 @@ private boolean shouldReverseLeafReaderContexts() {
     }
 
     // package-private for testing
-    LeafSlice[] slicesInternal(List<LeafReaderContext> leaves, int target_max_slices) {
+    LeafSlice[] slicesInternal(List<LeafReaderContext> leaves, int targetMaxSlice) {
         LeafSlice[] leafSlices;
-        if (target_max_slices <= 0) {
+        if (targetMaxSlice <= 0) {
             // use the default lucene slice calculation
             leafSlices = super.slices(leaves);
             logger.debug("Slice count using lucene default [{}]", leafSlices.length);
         } else {
-            // use the custom slice calculation based on target_max_slices. It will sort
-            leafSlices = MaxTargetSliceSupplier.getSlices(leaves, target_max_slices);
+            // use the custom slice calculation based on targetMaxSlice
+            leafSlices = MaxTargetSliceSupplier.getSlices(leaves, targetMaxSlice);
             logger.debug("Slice count using max target slice supplier [{}]", leafSlices.length);
         }
         return leafSlices;

server/src/main/java/org/opensearch/search/internal/MaxTargetSliceSupplier.java

@@ -21,16 +21,18 @@
  * all the leaves based on document count and then assign each leaf in round-robin fashion to the target slice count slices. Based on
  * experiment results as shared in <a href=https://github.com/opensearch-project/OpenSearch/issues/7358>issue-7358</a>
  * we can see this mechanism helps to achieve better tail/median latency over default lucene slice computation.
+ *
+ * @opensearch.internal
  */
 public class MaxTargetSliceSupplier {
 
-    public static IndexSearcher.LeafSlice[] getSlices(List<LeafReaderContext> leaves, int target_max_slice) {
-        if (target_max_slice <= 0) {
-            throw new IllegalArgumentException("MaxTargetSliceSupplier called with unexpected slice count of " + target_max_slice);
+    public static IndexSearcher.LeafSlice[] getSlices(List<LeafReaderContext> leaves, int targetMaxSlice) {
+        if (targetMaxSlice <= 0) {
+            throw new IllegalArgumentException("MaxTargetSliceSupplier called with unexpected slice count of " + targetMaxSlice);
         }
 
         // slice count should not exceed the segment count
-        int target_slice_count = Math.min(target_max_slice, leaves.size());
+        int targetSliceCount = Math.min(targetMaxSlice, leaves.size());
 
         // Make a copy so we can sort:
         List<LeafReaderContext> sortedLeaves = new ArrayList<>(leaves);
@@ -39,23 +41,15 @@ public static IndexSearcher.LeafSlice[] getSlices(List<LeafReaderContext> leaves
         sortedLeaves.sort(Collections.reverseOrder(Comparator.comparingInt(l -> l.reader().maxDoc())));
 
         final List<List<LeafReaderContext>> groupedLeaves = new ArrayList<>();
-        for (int i = 0; i < target_slice_count; ++i) {
+        for (int i = 0; i < targetSliceCount; ++i) {
             groupedLeaves.add(new ArrayList<>());
         }
         // distribute the slices in round-robin fashion
-        List<LeafReaderContext> group;
         for (int idx = 0; idx < sortedLeaves.size(); ++idx) {
-            int currentGroup = idx % target_slice_count;
-            group = groupedLeaves.get(currentGroup);
-            group.add(sortedLeaves.get(idx));
+            int currentGroup = idx % targetSliceCount;
+            groupedLeaves.get(currentGroup).add(sortedLeaves.get(idx));
         }
 
-        IndexSearcher.LeafSlice[] slices = new IndexSearcher.LeafSlice[target_slice_count];
-        int upto = 0;
-        for (List<LeafReaderContext> currentLeaf : groupedLeaves) {
-            slices[upto] = new IndexSearcher.LeafSlice(currentLeaf);
-            ++upto;
-        }
-        return slices;
+        return groupedLeaves.stream().map(IndexSearcher.LeafSlice::new).toArray(IndexSearcher.LeafSlice[]::new);
     }
 }

test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java

@@ -212,10 +212,7 @@ protected final Collection<Class<? extends Plugin>> pluginList(Class<? extends P
 
     /** Additional settings to add when creating the node. Also allows overriding the default settings. */
     protected Settings nodeSettings() {
-        // By default, for tests we will put the target slice count of 2. This will increase the probability of having multiple slices
-        // when tests are run with concurrent segment search enabled. When concurrent segment search is disabled then it's a no-op as
-        // slices are not used
-        return Settings.builder().put(SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, 2).build();
+        return Settings.EMPTY;
     }
 
     /** True if a dummy http transport should be used, or false if the real http transport should be used. */
@@ -251,6 +248,10 @@ private Node newNode() {
             .putList(INITIAL_CLUSTER_MANAGER_NODES_SETTING.getKey(), nodeName)
             .put(FeatureFlags.TELEMETRY_SETTING.getKey(), true)
             .put(TelemetrySettings.TRACER_ENABLED_SETTING.getKey(), true)
+            // By default, for tests we will put the target slice count of 2. This will increase the probability of having multiple slices
+            // when tests are run with concurrent segment search enabled. When concurrent segment search is disabled then it's a no-op as
+            // slices are not used
+            .put(SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, 2)
             .put(nodeSettings()) // allow test cases to provide their own settings or override these
             .build();