From c58eb62c82782ea2f2c79cbba3f650bdde559722 Mon Sep 17 00:00:00 2001 From: Sorabh Hamirwasia Date: Wed, 26 Jul 2023 16:07:29 -0700 Subject: [PATCH] Address review comments Signed-off-by: Sorabh Hamirwasia --- .../java/org/opensearch/search/SearchBootstrapSettings.java | 5 +++-- .../opensearch/search/internal/ContextIndexSearcher.java | 2 +- .../opensearch/search/internal/MaxTargetSliceSupplier.java | 4 ++-- .../search/internal/ContextIndexSearcherTests.java | 6 +++++- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/SearchBootstrapSettings.java b/server/src/main/java/org/opensearch/search/SearchBootstrapSettings.java index c8903ec6a44b6..2e32d13db980e 100644 --- a/server/src/main/java/org/opensearch/search/SearchBootstrapSettings.java +++ b/server/src/main/java/org/opensearch/search/SearchBootstrapSettings.java @@ -20,13 +20,14 @@ public class SearchBootstrapSettings { // settings to configure maximum slice created per search request using OS custom slice computation mechanism. Default lucene // mechanism will not be used if this setting is set with value > 0 public static final String CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY = "search.concurrent.max_slice"; - public static final int CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE = -1; + public static final int CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE = 0; - // value <= 0 means lucene slice computation will be used + // 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 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, + CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE, Setting.Property.NodeScope ); private static Settings settings; diff --git a/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java b/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java index bf733e9f5a00d..4d43f2c2c92a4 100644 --- a/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java +++ b/server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java @@ -539,7 +539,7 @@ private boolean shouldReverseLeafReaderContexts() { // package-private for testing LeafSlice[] slicesInternal(List leaves, int targetMaxSlice) { LeafSlice[] leafSlices; - if (targetMaxSlice <= 0) { + if (targetMaxSlice == 0) { // use the default lucene slice calculation leafSlices = super.slices(leaves); logger.debug("Slice count using lucene default [{}]", leafSlices.length); diff --git a/server/src/main/java/org/opensearch/search/internal/MaxTargetSliceSupplier.java b/server/src/main/java/org/opensearch/search/internal/MaxTargetSliceSupplier.java index 45feec67a7804..4b20ae6e771ea 100644 --- a/server/src/main/java/org/opensearch/search/internal/MaxTargetSliceSupplier.java +++ b/server/src/main/java/org/opensearch/search/internal/MaxTargetSliceSupplier.java @@ -24,9 +24,9 @@ * * @opensearch.internal */ -public class MaxTargetSliceSupplier { +final class MaxTargetSliceSupplier { - public static IndexSearcher.LeafSlice[] getSlices(List leaves, int targetMaxSlice) { + static IndexSearcher.LeafSlice[] getSlices(List leaves, int targetMaxSlice) { if (targetMaxSlice <= 0) { throw new IllegalArgumentException("MaxTargetSliceSupplier called with unexpected slice count of " + targetMaxSlice); } diff --git a/server/src/test/java/org/opensearch/search/internal/ContextIndexSearcherTests.java b/server/src/test/java/org/opensearch/search/internal/ContextIndexSearcherTests.java index b373b145b97ff..e971c60d673cf 100644 --- a/server/src/test/java/org/opensearch/search/internal/ContextIndexSearcherTests.java +++ b/server/src/test/java/org/opensearch/search/internal/ContextIndexSearcherTests.java @@ -82,6 +82,7 @@ import org.opensearch.index.cache.bitset.BitsetFilterCache; import org.opensearch.core.index.shard.ShardId; import org.opensearch.index.shard.IndexShard; +import org.opensearch.search.SearchBootstrapSettings; import org.opensearch.search.aggregations.LeafBucketCollector; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.IndexSettingsModule; @@ -332,7 +333,10 @@ public void testSlicesInternal() throws Exception { searchContext ); // Case 1: Verify the slice count when lucene default slice computation is used - IndexSearcher.LeafSlice[] slices = searcher.slicesInternal(leaves, -1); + IndexSearcher.LeafSlice[] slices = searcher.slicesInternal( + leaves, + SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE + ); int expectedSliceCount = 2; // 2 slices will be created since max segment per slice of 5 will be reached assertEquals(expectedSliceCount, slices.length);