From 75a553bcc6435d468e4c299169f0a1880d3971af 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 --- .../common/settings/ClusterSettings.java | 6 ++-- .../search/SearchBootstrapSettings.java | 5 +-- .../search/internal/ContextIndexSearcher.java | 2 +- .../internal/MaxTargetSliceSupplier.java | 4 +-- .../common/settings/SettingsModuleTests.java | 33 +++++++++++++++++++ .../internal/ContextIndexSearcherTests.java | 6 +++- 6 files changed, 48 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java index cae0aa0ede83b..f494cf91e0db9 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -495,7 +495,6 @@ public void apply(Settings value, Settings current, Settings previous) { SearchService.MAX_OPEN_SCROLL_CONTEXT, SearchService.MAX_OPEN_PIT_CONTEXT, SearchService.MAX_PIT_KEEPALIVE_SETTING, - SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING, CreatePitController.PIT_INIT_KEEP_ALIVE, Node.WRITE_PORTS_FILE_SETTING, Node.NODE_NAME_SETTING, @@ -679,7 +678,10 @@ public void apply(Settings value, Settings current, Settings previous) { IndicesService.CLUSTER_REMOTE_TRANSLOG_REPOSITORY_SETTING ), List.of(FeatureFlags.CONCURRENT_SEGMENT_SEARCH), - List.of(SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING), + List.of( + SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING, + SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING + ), List.of(FeatureFlags.TELEMETRY), List.of(TelemetrySettings.TRACER_ENABLED_SETTING) ); 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/common/settings/SettingsModuleTests.java b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java index 422d39c5e706d..bdace194a84d3 100644 --- a/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java +++ b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java @@ -37,6 +37,7 @@ import org.hamcrest.Matchers; import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.IndexSettings; +import org.opensearch.search.SearchBootstrapSettings; import org.opensearch.search.SearchService; import org.opensearch.test.FeatureFlagSetter; @@ -335,4 +336,36 @@ public void testConcurrentSegmentSearchIndexSettings() { "node" ); } + + public void testMaxSliceCountClusterSettingsForConcurrentSearch() { + // Test that we throw an exception without the feature flag + Settings settings = Settings.builder() + .put(SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING.getKey(), true) + .build(); + IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> new SettingsModule(settings)); + assertTrue( + ex.getMessage() + .contains("unknown setting [" + SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING.getKey()) + ); + + // Test that the settings updates correctly with the feature flag + FeatureFlagSetter.set(FeatureFlags.CONCURRENT_SEGMENT_SEARCH); + int settingValue = randomIntBetween(0, 10); + Settings settingsWithFeatureFlag = Settings.builder() + .put(SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING.getKey(), settingValue) + .build(); + SettingsModule settingsModule = new SettingsModule(settingsWithFeatureFlag); + assertEquals( + settingValue, + (int) SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING.get(settingsModule.getSettings()) + ); + + // Test that negative value is not allowed + settingValue = -1; + final Settings settingsWithFeatureFlag_2 = Settings.builder() + .put(SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING.getKey(), settingValue) + .build(); + ex = expectThrows(IllegalArgumentException.class, () -> new SettingsModule(settingsWithFeatureFlag_2)); + assertTrue(ex.getMessage().contains(SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING.getKey())); + } } 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);