From fe7000df4798bc369efad6a6f0ccecf80f3bc20d Mon Sep 17 00:00:00 2001 From: Neetika Singhal Date: Tue, 15 Aug 2023 16:54:36 -0700 Subject: [PATCH] Rethrow OpenSearch exception in concurrent search (#9177) Signed-off-by: Neetika Singhal Signed-off-by: Kiran Reddy --- CHANGELOG.md | 1 + .../breaker/CircuitBreakerServiceIT.java | 35 +++++++++++++++++-- .../query/ConcurrentQueryPhaseSearcher.java | 5 +-- 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1f2e661ddadb..7af3c171e8c6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -143,6 +143,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Refactor] Task foundation classes to core library - pt 1 ([#9082](https://github.com/opensearch-project/OpenSearch/pull/9082)) - Add base class for parameterizing the search based tests #9083 ([#9083](https://github.com/opensearch-project/OpenSearch/pull/9083)) - Add support for wrapping CollectorManager with profiling during concurrent execution ([#9129](https://github.com/opensearch-project/OpenSearch/pull/9129)) +- Rethrow OpenSearch exception for non-concurrent path while using concurrent search ([#9177](https://github.com/opensearch-project/OpenSearch/pull/9177)) ### Deprecated 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 10bd179ddc5fd..a7c7ee1ae8a4d 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 @@ -32,6 +32,7 @@ package org.opensearch.indices.memory.breaker; +import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; import org.opensearch.ExceptionsHelper; import org.opensearch.action.admin.cluster.node.stats.NodeStats; import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; @@ -46,6 +47,7 @@ import org.opensearch.client.Requests; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.routing.allocation.decider.EnableAllocationDecider; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.core.common.breaker.NoopCircuitBreaker; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; @@ -57,14 +59,17 @@ import org.opensearch.core.xcontent.MediaTypeRegistry; import org.opensearch.core.rest.RestStatus; import org.opensearch.indices.breaker.HierarchyCircuitBreakerService; +import org.opensearch.search.SearchService; import org.opensearch.search.sort.SortOrder; -import org.opensearch.test.OpenSearchIntegTestCase; import org.opensearch.test.OpenSearchIntegTestCase.ClusterScope; import org.junit.After; import org.junit.Before; +import org.opensearch.test.ParameterizedOpenSearchIntegTestCase; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.concurrent.TimeUnit; @@ -73,6 +78,7 @@ import static org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest.Metric.BREAKER; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_REPLICAS; import static org.opensearch.index.query.QueryBuilders.matchAllQuery; +import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING; import static org.opensearch.search.aggregations.AggregationBuilders.cardinality; import static org.opensearch.search.aggregations.AggregationBuilders.terms; import static org.opensearch.test.OpenSearchIntegTestCase.Scope.TEST; @@ -89,7 +95,32 @@ * Integration tests for InternalCircuitBreakerService */ @ClusterScope(scope = TEST, numClientNodes = 0, maxNumDataNodes = 1) -public class CircuitBreakerServiceIT extends OpenSearchIntegTestCase { +public class CircuitBreakerServiceIT extends ParameterizedOpenSearchIntegTestCase { + public CircuitBreakerServiceIT(Settings dynamicSettings) { + super(dynamicSettings); + } + + @ParametersFactory + public static Collection parameters() { + return Arrays.asList( + new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), false).build() }, + new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).build() } + ); + } + + @Override + protected Settings featureFlagSettings() { + return Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.CONCURRENT_SEGMENT_SEARCH, "true").build(); + } + + @Override + 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)) + .build(); + } + /** Reset all breaker settings back to their defaults */ private void reset() { logger.info("--> resetting breaker settings"); diff --git a/server/src/main/java/org/opensearch/search/query/ConcurrentQueryPhaseSearcher.java b/server/src/main/java/org/opensearch/search/query/ConcurrentQueryPhaseSearcher.java index 4d3b2a124f64f..e22f766d3894c 100644 --- a/server/src/main/java/org/opensearch/search/query/ConcurrentQueryPhaseSearcher.java +++ b/server/src/main/java/org/opensearch/search/query/ConcurrentQueryPhaseSearcher.java @@ -13,6 +13,7 @@ import org.apache.lucene.search.Collector; import org.apache.lucene.search.CollectorManager; import org.apache.lucene.search.Query; +import org.opensearch.OpenSearchException; import org.opensearch.search.aggregations.AggregationProcessor; import org.opensearch.search.aggregations.ConcurrentAggregationProcessor; import org.opensearch.search.internal.ContextIndexSearcher; @@ -103,8 +104,8 @@ public AggregationProcessor aggregationProcessor(SearchContext searchContext) { } private static void rethrowCauseIfPossible(RuntimeException re, SearchContext searchContext) throws T { - // Rethrow exception if cause is null - if (re.getCause() == null) { + // Rethrow exception if cause is null or if it's an instance of OpenSearchException + if (re.getCause() == null || re instanceof OpenSearchException) { throw re; }