From 252b2d8073008bb1c675e68a84abc02a6709c34c Mon Sep 17 00:00:00 2001 From: Vikasht34 Date: Thu, 24 Apr 2025 12:58:35 -0700 Subject: [PATCH] Enable concurrent_segment_search auto mode by default (#17978) * Enable concurrent_segment_search auto mode by default Signed-off-by: Vikasht34 * Make Default Slice count to 1 for Non-Concurrent Path Signed-off-by: Vikasht34 * Add tolerance to matrix_stats agg correlation value assertion The correlation metric could be different for different document distribution across shards, or slices. slice1(doc1,doc2), slice2(doc3,doc4,doc5) could give different correlation from slice1(doc1,doc2,doc3), slice2(doc4,doc5) The tolerance followed here is 0.000000000000001 Signed-off-by: bowenlan-amzn --------- Signed-off-by: Vikasht34 Signed-off-by: bowenlan-amzn Co-authored-by: bowenlan-amzn --- CHANGELOG.md | 1 + .../test/stats/30_single_value_field.yml | 6 +- .../test/stats/40_multi_value_field.yml | 12 ++- .../breaker/CircuitBreakerServiceIT.java | 2 +- .../search/stats/ConcurrentSearchStatsIT.java | 2 +- .../org/opensearch/index/IndexSettings.java | 7 +- .../search/DefaultSearchContext.java | 91 +++++++++++++------ .../org/opensearch/search/SearchService.java | 35 +++++-- .../internal/ContextIndexSearcherTests.java | 2 +- .../test/OpenSearchIntegTestCase.java | 2 +- .../test/OpenSearchSingleNodeTestCase.java | 2 +- 11 files changed, 112 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a34eeb5be7d5c..101a3718d6161 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Changed - Change the default max header size from 8KB to 16KB. ([#18024](https://github.com/opensearch-project/OpenSearch/pull/18024)) +- Enable concurrent_segment_search auto mode by default[#17978](https://github.com/opensearch-project/OpenSearch/pull/17978) ### Dependencies diff --git a/modules/aggs-matrix-stats/src/yamlRestTest/resources/rest-api-spec/test/stats/30_single_value_field.yml b/modules/aggs-matrix-stats/src/yamlRestTest/resources/rest-api-spec/test/stats/30_single_value_field.yml index 77e8bf6359f22..c18b1e5fc0448 100644 --- a/modules/aggs-matrix-stats/src/yamlRestTest/resources/rest-api-spec/test/stats/30_single_value_field.yml +++ b/modules/aggs-matrix-stats/src/yamlRestTest/resources/rest-api-spec/test/stats/30_single_value_field.yml @@ -145,7 +145,8 @@ setup: - match: {hits.total: 15} - match: {aggregations.mfs.doc_count: 14} - match: {aggregations.mfs.fields.0.count: 14} - - match: {aggregations.mfs.fields.2.correlation.val2: 0.9569513137793205} + - gte: {aggregations.mfs.fields.2.correlation.val2: 0.956951313779319} + - lte: {aggregations.mfs.fields.2.correlation.val2: 0.956951313779321} --- "Partially unmapped with missing default": @@ -159,7 +160,8 @@ setup: - match: {hits.total: 15} - match: {aggregations.mfs.doc_count: 15} - match: {aggregations.mfs.fields.0.count: 15} - - match: {aggregations.mfs.fields.2.correlation.val2: 0.9567970467908384} + - gte: {aggregations.mfs.fields.2.correlation.val2: 0.956797046790837} + - lte: {aggregations.mfs.fields.2.correlation.val2: 0.956797046790839} --- "With script": diff --git a/modules/aggs-matrix-stats/src/yamlRestTest/resources/rest-api-spec/test/stats/40_multi_value_field.yml b/modules/aggs-matrix-stats/src/yamlRestTest/resources/rest-api-spec/test/stats/40_multi_value_field.yml index 467efce78a467..218976c3fbbdf 100644 --- a/modules/aggs-matrix-stats/src/yamlRestTest/resources/rest-api-spec/test/stats/40_multi_value_field.yml +++ b/modules/aggs-matrix-stats/src/yamlRestTest/resources/rest-api-spec/test/stats/40_multi_value_field.yml @@ -132,7 +132,8 @@ setup: - match: {hits.total: 15} - match: {aggregations.mfs.doc_count: 14} - match: {aggregations.mfs.fields.0.count: 14} - - match: {aggregations.mfs.fields.0.correlation.val1: 0.06838646533369998} + - gte: {aggregations.mfs.fields.0.correlation.val1: 0.068386465333698} + - lte: {aggregations.mfs.fields.0.correlation.val1: 0.068386465333701} --- "Multi value field Min": @@ -146,7 +147,8 @@ setup: - match: {hits.total: 15} - match: {aggregations.mfs.doc_count: 14} - match: {aggregations.mfs.fields.0.count: 14} - - match: {aggregations.mfs.fields.0.correlation.val1: -0.09777682707831963} + - gte: {aggregations.mfs.fields.0.correlation.val1: -0.097776827078320} + - lte: {aggregations.mfs.fields.0.correlation.val1: -0.097776827078318} --- "Partially unmapped": @@ -160,7 +162,8 @@ setup: - match: {hits.total: 15} - match: {aggregations.mfs.doc_count: 13} - match: {aggregations.mfs.fields.0.count: 13} - - match: {aggregations.mfs.fields.0.correlation.val1: -0.044997535185684244} + - gte: {aggregations.mfs.fields.0.correlation.val1: -0.044997535185685} + - lte: {aggregations.mfs.fields.0.correlation.val1: -0.044997535185683} --- "Partially unmapped with missing defaults": @@ -174,7 +177,8 @@ setup: - match: {hits.total: 15} - match: {aggregations.mfs.doc_count: 15} - match: {aggregations.mfs.fields.0.count: 15} - - match: {aggregations.mfs.fields.0.correlation.val2: 0.04028024709708195} + - gte: {aggregations.mfs.fields.0.correlation.val2: 0.040280247097080} + - lte: {aggregations.mfs.fields.0.correlation.val2: 0.040280247097082} --- "With script": diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerServiceIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerServiceIT.java index 441259c3ba41a..a255ce40f9c1b 100644 --- a/server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerServiceIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerServiceIT.java @@ -110,7 +110,7 @@ public static Collection parameters() { protected Settings nodeSettings(int nodeOrdinal) { return Settings.builder() .put(super.nodeSettings(nodeOrdinal)) - .put(SearchService.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, randomIntBetween(1, 2)) + .put(SearchService.CONCURRENT_SEGMENT_SEARCH_MAX_SLICE_COUNT_KEY, randomIntBetween(1, 2)) .build(); } diff --git a/server/src/internalClusterTest/java/org/opensearch/search/stats/ConcurrentSearchStatsIT.java b/server/src/internalClusterTest/java/org/opensearch/search/stats/ConcurrentSearchStatsIT.java index f8d2955440bc4..eb93abf7c38f2 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/stats/ConcurrentSearchStatsIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/stats/ConcurrentSearchStatsIT.java @@ -59,7 +59,7 @@ protected Settings nodeSettings(int nodeOrdinal) { .put(super.nodeSettings(nodeOrdinal)) .put(IndicesService.INDICES_CACHE_CLEAN_INTERVAL_SETTING.getKey(), "1ms") .put(IndicesQueryCache.INDICES_QUERIES_CACHE_ALL_SEGMENTS_SETTING.getKey(), true) - .put(SearchService.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, SEGMENT_SLICE_COUNT) + .put(SearchService.CONCURRENT_SEGMENT_SEARCH_MAX_SLICE_COUNT_KEY, SEGMENT_SLICE_COUNT) .put(SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true) .build(); } diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index 578304c77fef9..26a4d9d8469a3 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/server/src/main/java/org/opensearch/index/IndexSettings.java @@ -78,10 +78,11 @@ import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_NESTED_FIELDS_LIMIT_SETTING; import static org.opensearch.index.mapper.MapperService.INDEX_MAPPING_TOTAL_FIELDS_LIMIT_SETTING; import static org.opensearch.index.store.remote.directory.RemoteSnapshotDirectory.SEARCHABLE_SNAPSHOT_EXTENDED_COMPATIBILITY_MINIMUM_VERSION; +import static org.opensearch.search.SearchService.CONCURRENT_SEGMENT_SEARCH_DEFAULT_SLICE_COUNT_VALUE; +import static org.opensearch.search.SearchService.CONCURRENT_SEGMENT_SEARCH_MIN_SLICE_COUNT_VALUE; import static org.opensearch.search.SearchService.CONCURRENT_SEGMENT_SEARCH_MODE_ALL; import static org.opensearch.search.SearchService.CONCURRENT_SEGMENT_SEARCH_MODE_AUTO; import static org.opensearch.search.SearchService.CONCURRENT_SEGMENT_SEARCH_MODE_NONE; -import static org.opensearch.search.SearchService.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE; /** * This class encapsulates all index level settings and handles settings updates. @@ -726,8 +727,8 @@ public static IndexMergePolicy fromString(String text) { public static final Setting INDEX_CONCURRENT_SEGMENT_SEARCH_MAX_SLICE_COUNT = Setting.intSetting( "index.search.concurrent.max_slice_count", - CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE, - CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE, + CONCURRENT_SEGMENT_SEARCH_DEFAULT_SLICE_COUNT_VALUE, + CONCURRENT_SEGMENT_SEARCH_MIN_SLICE_COUNT_VALUE, Property.Dynamic, Property.IndexScope ); diff --git a/server/src/main/java/org/opensearch/search/DefaultSearchContext.java b/server/src/main/java/org/opensearch/search/DefaultSearchContext.java index 854a8dee56e6d..a913f943cd19c 100644 --- a/server/src/main/java/org/opensearch/search/DefaultSearchContext.java +++ b/server/src/main/java/org/opensearch/search/DefaultSearchContext.java @@ -50,6 +50,8 @@ import org.opensearch.common.SetOnce; import org.opensearch.common.lease.Releasables; import org.opensearch.common.lucene.search.Queries; +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.BigArrays; import org.opensearch.core.concurrency.OpenSearchRejectedExecutionException; @@ -1056,46 +1058,77 @@ public BucketCollectorProcessor bucketCollectorProcessor() { } /** - * Evaluate the concurrentSearchMode based on cluster and index settings if concurrent segment search - * should be used for this request context - * If the cluster.search.concurrent_segment_search.mode setting - * is not explicitly set, the evaluation falls back to the - * cluster.search.concurrent_segment_search.enabled boolean setting - * which will evaluate to true or false. This is then evaluated to "all" or "none" respectively - * @return one of "none", "auto", "all" + * Determines the appropriate concurrent segment search mode for the current search request. + *

+ * This method evaluates both index-level and cluster-level settings to decide whether + * concurrent segment search should be enabled. The resolution logic is as follows: + *

    + *
  1. If the request targets a system index or uses search throttling, concurrent segment search is disabled.
    2. + *
  2. If a legacy boolean setting is present (cluster or index level), it is honored: + *
      + *
    • true → enables concurrent segment search ("all")
      • + *
    • false → disables it ("none")
      • + *
    + *
    3. + *
  3. Otherwise, the modern string-based setting is used. Allowed values are: "none", "auto", or "all".
    4. + *
+ * + * @param concurrentSearchExecutor the executor used for concurrent segment search; if null, disables the feature + * @return the resolved concurrent segment search mode: "none", "auto", or "all" */ private String evaluateConcurrentSearchMode(Executor concurrentSearchExecutor) { - // Do not use concurrent segment search for system indices or throttled requests. See: - // https://github.com/opensearch-project/OpenSearch/issues/12951 - if (indexShard.isSystem() || indexShard.indexSettings().isSearchThrottled()) { + // Skip concurrent search for system indices, throttled requests, or if dependencies are missing + if (indexShard.isSystem() + || indexShard.indexSettings().isSearchThrottled() + || clusterService == null + || concurrentSearchExecutor == null) { return CONCURRENT_SEGMENT_SEARCH_MODE_NONE; } - if ((clusterService != null) && concurrentSearchExecutor != null) { - String concurrentSearchMode = indexService.getIndexSettings() - .getSettings() - .get( - IndexSettings.INDEX_CONCURRENT_SEGMENT_SEARCH_MODE.getKey(), - clusterService.getClusterSettings().getOrNull(CLUSTER_CONCURRENT_SEGMENT_SEARCH_MODE) - ); - if (concurrentSearchMode != null) { - return concurrentSearchMode; - } - // mode setting not set, fallback to concurrent_segment_search.enabled setting - return indexService.getIndexSettings() - .getSettings() - .getAsBoolean( - IndexSettings.INDEX_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), - clusterService.getClusterSettings().get(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING) - ) ? CONCURRENT_SEGMENT_SEARCH_MODE_ALL : CONCURRENT_SEGMENT_SEARCH_MODE_NONE; + Settings indexSettings = indexService.getIndexSettings().getSettings(); + ClusterSettings clusterSettings = clusterService.getClusterSettings(); + final String newConcurrentMode = indexSettings.get( + IndexSettings.INDEX_CONCURRENT_SEGMENT_SEARCH_MODE.getKey(), + clusterSettings.getOrNull(CLUSTER_CONCURRENT_SEGMENT_SEARCH_MODE) + ); + + // Step 1: New concurrent mode is explicitly set → use it + if (newConcurrentMode != null) return newConcurrentMode; + + final Boolean legacySetting = indexSettings.getAsBoolean( + IndexSettings.INDEX_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), + clusterSettings.getOrNull(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING) + ); + + // Step 2: If new concurrent setting is not explicitly set + // then check if Legacy boolean setting is explicitly set → use it + if (legacySetting != null) { + return Boolean.TRUE.equals(legacySetting) ? CONCURRENT_SEGMENT_SEARCH_MODE_ALL : CONCURRENT_SEGMENT_SEARCH_MODE_NONE; } - return CONCURRENT_SEGMENT_SEARCH_MODE_NONE; + + // Step 3: Neither explicitly set → use default concurrent mode + return indexSettings.get( + IndexSettings.INDEX_CONCURRENT_SEGMENT_SEARCH_MODE.getKey(), + clusterSettings.get(CLUSTER_CONCURRENT_SEGMENT_SEARCH_MODE) + ); } + /** + * Returns the target maximum slice count to use for concurrent segment search. + * + * If concurrent segment search is disabled (either due to system index, throttled search, + * missing executor, or explicitly disabled settings), then we return a slice count of 1. + * This effectively disables concurrent slicing and ensures that the search is performed + * in a single-threaded manner. + * + * Otherwise, fetch the configured slice count from index or cluster-level settings. + * + * @return number of slices to use for concurrent segment search; returns 1 if concurrent search is disabled. + */ @Override public int getTargetMaxSliceCount() { if (shouldUseConcurrentSearch() == false) { - throw new IllegalStateException("Target slice count should not be used when concurrent search is disabled"); + return 1; // Disable slicing: run search in a single thread when concurrent search is off } return indexService.getIndexSettings() diff --git a/server/src/main/java/org/opensearch/search/SearchService.java b/server/src/main/java/org/opensearch/search/SearchService.java index 1fa0fca68fca1..d3b00ae8fb0c8 100644 --- a/server/src/main/java/org/opensearch/search/SearchService.java +++ b/server/src/main/java/org/opensearch/search/SearchService.java @@ -279,7 +279,7 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv public static final Setting CLUSTER_CONCURRENT_SEGMENT_SEARCH_MODE = Setting.simpleString( "search.concurrent_segment_search.mode", - CONCURRENT_SEGMENT_SEARCH_MODE_NONE, + CONCURRENT_SEGMENT_SEARCH_MODE_AUTO, value -> { switch (value) { case CONCURRENT_SEGMENT_SEARCH_MODE_ALL: @@ -297,18 +297,18 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv // 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_count"; - public static final int CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE = 0; + public static final String CONCURRENT_SEGMENT_SEARCH_MAX_SLICE_COUNT_KEY = "search.concurrent.max_slice_count"; + public static final int CONCURRENT_SEGMENT_SEARCH_DEFAULT_SLICE_COUNT_VALUE = computeDefaultSliceCount(); + public static final int CONCURRENT_SEGMENT_SEARCH_MIN_SLICE_COUNT_VALUE = 0; // value == 0 means lucene slice computation will be used 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, + CONCURRENT_SEGMENT_SEARCH_MAX_SLICE_COUNT_KEY, + CONCURRENT_SEGMENT_SEARCH_DEFAULT_SLICE_COUNT_VALUE, + CONCURRENT_SEGMENT_SEARCH_MIN_SLICE_COUNT_VALUE, Property.Dynamic, Property.NodeScope ); - // value 0 means rewrite filters optimization in aggregations will be disabled @ExperimentalApi public static final Setting MAX_AGGREGATION_REWRITE_FILTERS = Setting.intSetting( @@ -1871,6 +1871,27 @@ public MinAndMax estimatedMinAndMax() { } } + /** + * Computes the default maximum number of slices for concurrent segment search. + *

+ * This value is dynamically calculated as: + * 

+     *     min(availableProcessors / 2, 4)
+     *
+ * This ensures that: + *
    + *
  • On small machines, it avoids over-threading.
    • + *
  • On larger machines, it caps the concurrency to a reasonable level (4 slices).
    • + *
+ * This default is used when the user does not explicitly set the + * {@code search.concurrent.max_slice_count} cluster setting. + * + * @return the computed default slice count + */ + private static int computeDefaultSliceCount() { + return Math.max(1, Math.min(Runtime.getRuntime().availableProcessors() / 2, 4)); + } + /** * This helper class ensures we only execute either the success or the failure path for {@link SearchOperationListener}. * This is crucial for some implementations like {@link org.opensearch.index.search.stats.ShardSearchStats}. 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 4e02ee13a50e3..e645cd6a7723f 100644 --- a/server/src/test/java/org/opensearch/search/internal/ContextIndexSearcherTests.java +++ b/server/src/test/java/org/opensearch/search/internal/ContextIndexSearcherTests.java @@ -341,7 +341,7 @@ public void testSlicesInternal() throws Exception { // Case 1: Verify the slice count when lucene default slice computation is used IndexSearcher.LeafSlice[] slices = searcher.slicesInternal( leaves, - SearchService.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_DEFAULT_VALUE + SearchService.CONCURRENT_SEGMENT_SEARCH_MIN_SLICE_COUNT_VALUE ); int expectedSliceCount = 2; // 2 slices will be created since max segment per slice of 5 will be reached 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 6e47f7aa29345..3a5b2e84d81a5 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java @@ -1958,7 +1958,7 @@ protected Settings nodeSettings(int nodeOrdinal) { .putList(DISCOVERY_SEED_PROVIDERS_SETTING.getKey(), "file") // 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 - .put(SearchService.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, 2) + .put(SearchService.CONCURRENT_SEGMENT_SEARCH_MAX_SLICE_COUNT_KEY, 2) .put(featureFlagSettings()); // Enable tracer only when Telemetry Setting is enabled 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 a3d5166b23fb8..0879eacb938c1 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java @@ -257,7 +257,7 @@ private Node newNode() { .put(TelemetrySettings.TRACER_FEATURE_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 - .put(SearchService.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_KEY, 2) + .put(SearchService.CONCURRENT_SEGMENT_SEARCH_MAX_SLICE_COUNT_KEY, 2) .put(nodeSettings()) // allow test cases to provide their own settings or override these .put(featureFlagSettings);