From c1cbd7c6ce8b5106610c68348a607caae58f1aba 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 +++- .../test/OpenSearchIntegTestCase.java | 11 ++++--- .../test/OpenSearchSingleNodeTestCase.java | 17 +++++----- 8 files changed, 63 insertions(+), 21 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); diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java index 783428756b261..c88e87d68b704 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java @@ -1942,17 +1942,18 @@ protected Settings nodeSettings(int nodeOrdinal) { .put(SearchService.LOW_LEVEL_CANCELLATION_SETTING.getKey(), randomBoolean()) .putList(DISCOVERY_SEED_HOSTS_SETTING.getKey()) // empty list disables a port scan for other nodes .putList(DISCOVERY_SEED_PROVIDERS_SETTING.getKey(), "file") - .put(featureFlagSettings()) - // 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(featureFlagSettings()); if (rarely()) { // Sometimes adjust the minimum search thread pool size, causing // QueueResizingOpenSearchThreadPoolExecutor to be used instead of a regular // fixed thread pool builder.put("thread_pool.search.min_queue_size", 100); } + if (FeatureFlags.isEnabled(FeatureFlags.CONCURRENT_SEGMENT_SEARCH)) { + // 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 + builder.put(SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, 2); + } return builder.build(); } diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java index 1c999acb2827d..a07843f22ec00 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java @@ -224,7 +224,7 @@ private Node newNode() { final Path tempDir = createTempDir(); final String nodeName = nodeSettings().get(Node.NODE_NAME_SETTING.getKey(), "node_s_0"); - Settings settings = Settings.builder() + Settings.Builder settingsBuilder = Settings.builder() .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", random().nextLong())) .put(Environment.PATH_HOME_SETTING.getKey(), tempDir) .put(Environment.PATH_REPO_SETTING.getKey(), tempDir.resolve("repo")) @@ -247,13 +247,14 @@ private Node newNode() { .putList(DISCOVERY_SEED_HOSTS_SETTING.getKey()) // empty list disables a port scan for other nodes .putList(INITIAL_CLUSTER_MANAGER_NODES_SETTING.getKey(), nodeName) .put(FeatureFlags.TELEMETRY_SETTING.getKey(), true) - .put(TelemetrySettings.TRACER_ENABLED_SETTING.getKey(), true) + .put(TelemetrySettings.TRACER_ENABLED_SETTING.getKey(), true); + if (FeatureFlags.isEnabled(FeatureFlags.CONCURRENT_SEGMENT_SEARCH)) { // 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(); + // when tests are run with concurrent segment search enabled + settingsBuilder.put(SearchBootstrapSettings.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, 2); + } + // allow test cases to provide their own settings or override these + settingsBuilder.put(nodeSettings()); Collection> plugins = getPlugins(); if (plugins.contains(getTestTransportPlugin()) == false) { @@ -265,7 +266,7 @@ private Node newNode() { } plugins.add(MockScriptService.TestPlugin.class); plugins.add(MockTelemetryPlugin.class); - Node node = new MockNode(settings, plugins, forbidPrivateIndexSettings()); + Node node = new MockNode(settingsBuilder.build(), plugins, forbidPrivateIndexSettings()); try { node.start(); } catch (NodeValidationException e) {