Skip to content

Commit

Permalink
Introduce dynamic cluster setting to disable concurrent segment searc…
Browse files Browse the repository at this point in the history 
…h for given types of aggregations

Also refactored parent-join search ITs to support concurrent search parameterization and introduced a new test to cover concurrent search failures.

Signed-off-by: Jay Deng <jayd0104@gmail.com>
  • Loading branch information
@jed326
jed326 authored and Jay Deng committed Aug 21, 2023
1 parent 61c5f17 commit 2f2cfd7
Show file tree
Hide file tree
Showing 11 changed files with 220 additions and 3 deletions.
48 changes: 48 additions & 0 deletions ...nternalClusterTest/java/org/opensearch/join/aggregations/AbstractParentChildTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@
package org.opensearch.join.aggregations;

import org.opensearch.action.index.IndexRequestBuilder;
import org.opensearch.action.search.SearchRequestBuilder;
import org.opensearch.action.search.SearchResponse;
import org.opensearch.client.Requests;
import org.opensearch.cluster.metadata.IndexMetadata;
import org.opensearch.common.settings.Settings;
import org.opensearch.join.query.ParentChildTestCase;
import org.junit.Before;

Expand All @@ -44,6 +49,7 @@
import java.util.Set;

import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertSearchResponse;

/**
* Small base test-class which combines stuff used for Children and Parent aggregation tests
Expand All @@ -52,6 +58,10 @@ public abstract class AbstractParentChildTestCase extends ParentChildTestCase {
protected final Map<String, Control> categoryToControl = new HashMap<>();
protected final Map<String, ParentControl> articleToControl = new HashMap<>();

public AbstractParentChildTestCase(Settings dynamicSettings) {
super(dynamicSettings);
}

@Before
public void setupCluster() throws Exception {
assertAcked(
Expand Down Expand Up @@ -154,4 +164,42 @@ private ParentControl(String category) {
this.category = category;
}
}

// Test when there is 1 child document and 1 parent document per segment.
public void testSparseSegments() throws InterruptedException {
assertAcked(
prepareCreate("sparse").setMapping(
addFieldMappings(
buildParentJoinFieldMappingFromSimplifiedDef("join_field", true, "article", "comment"),
"commenter",
"keyword",
"category",
"keyword"
)
).setSettings(Settings.builder().put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1).put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0))
);

List<IndexRequestBuilder> requests = new ArrayList<>();
requests.add(createIndexRequest("sparse", "article", "article-0", null, "category", List.of("0")));
indexRandom(true,false, requests);
client().admin().indices().refresh(Requests.refreshRequest("sparse")).actionGet();
requests = new ArrayList<>();
requests.add(createIndexRequest(
"sparse",
"comment",
"comment-0",
"article-0",
"commenter",
"0"
));
indexRandom(true, false, requests);

SearchResponse searchResponse = getSearchRequest().get();
assertSearchResponse(searchResponse);
validateSpareSegmentsSearchResponse(searchResponse);
}

abstract SearchRequestBuilder getSearchRequest();

abstract void validateSpareSegmentsSearchResponse(SearchResponse searchResponse);
}
40 changes: 40 additions & 0 deletions ...parent-join/src/internalClusterTest/java/org/opensearch/join/aggregations/ChildrenIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,16 @@

package org.opensearch.join.aggregations;

import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
import org.apache.lucene.search.join.ScoreMode;
import org.opensearch.action.index.IndexRequestBuilder;
import org.opensearch.action.search.SearchRequestBuilder;
import org.opensearch.action.search.SearchResponse;
import org.opensearch.action.update.UpdateResponse;
import org.opensearch.client.Requests;
import org.opensearch.cluster.metadata.IndexMetadata;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.search.SearchHit;
import org.opensearch.search.aggregations.AggregationBuilders;
import org.opensearch.search.aggregations.InternalAggregation;
Expand All @@ -47,14 +50,19 @@
import org.opensearch.search.sort.SortOrder;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Set;

import static org.opensearch.index.query.QueryBuilders.matchAllQuery;
import static org.opensearch.index.query.QueryBuilders.matchQuery;
import static org.opensearch.index.query.QueryBuilders.termQuery;
import static org.opensearch.join.aggregations.JoinAggregationBuilders.children;
import static org.opensearch.join.aggregations.JoinAggregationBuilders.parent;
import static org.opensearch.join.query.JoinQueryBuilders.hasChildQuery;
import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
import static org.opensearch.search.aggregations.AggregationBuilders.sum;
import static org.opensearch.search.aggregations.AggregationBuilders.terms;
import static org.opensearch.search.aggregations.AggregationBuilders.topHits;
Expand All @@ -69,6 +77,23 @@

public class ChildrenIT extends AbstractParentChildTestCase {

public ChildrenIT(Settings settings) {
super(settings);
}

@ParametersFactory
public static Collection<Object[]> 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();
}

public void testChildrenAggs() throws Exception {
SearchResponse searchResponse = client().prepareSearch("test")
.setQuery(matchQuery("randomized", true))
Expand Down Expand Up @@ -407,4 +432,19 @@ public void testPostCollectAllLeafReaders() throws Exception {
children = parents.getBuckets().get(0).getAggregations().get("child_docs");
assertThat(children.getDocCount(), equalTo(2L));
}


@Override
SearchRequestBuilder getSearchRequest() {
return client().prepareSearch("sparse")
.setSize(10000)
.setQuery(matchAllQuery())
.addAggregation(children("to_comment", "comment").subAggregation(terms("commenters").field("commenter").size(10000)));
}

@Override
void validateSpareSegmentsSearchResponse(SearchResponse searchResponse) {
Children children = searchResponse.getAggregations().get("to_comment");
assertEquals(children.getDocCount(), 1);
}
}
39 changes: 39 additions & 0 deletions ...s/parent-join/src/internalClusterTest/java/org/opensearch/join/aggregations/ParentIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,17 @@

package org.opensearch.join.aggregations;

import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
import org.opensearch.action.search.SearchRequestBuilder;
import org.opensearch.action.search.SearchResponse;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.search.aggregations.Aggregation;
import org.opensearch.search.aggregations.bucket.MultiBucketsAggregation;
import org.opensearch.search.aggregations.bucket.terms.Terms;

import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand All @@ -47,15 +52,34 @@
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.opensearch.index.query.QueryBuilders.matchAllQuery;
import static org.opensearch.index.query.QueryBuilders.matchQuery;
import static org.opensearch.join.aggregations.JoinAggregationBuilders.parent;
import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
import static org.opensearch.search.aggregations.AggregationBuilders.terms;
import static org.opensearch.search.aggregations.AggregationBuilders.topHits;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertSearchResponse;
import static org.hamcrest.Matchers.equalTo;

public class ParentIT extends AbstractParentChildTestCase {

public ParentIT(Settings settings) {
super(settings);
}

@ParametersFactory
public static Collection<Object[]> 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();
}

public void testSimpleParentAgg() throws Exception {
final SearchRequestBuilder searchRequest = client().prepareSearch("test")
.setSize(10000)
Expand Down Expand Up @@ -264,4 +288,19 @@ public void testTermsParentAggTerms() throws Exception {
}
}
}


@Override
SearchRequestBuilder getSearchRequest() {
return client().prepareSearch("sparse")
.setSize(10000)
.setQuery(matchAllQuery())
.addAggregation(parent("to_article", "comment").subAggregation(terms("category").field("category").size(10000)));
}

@Override
void validateSpareSegmentsSearchResponse(SearchResponse searchResponse) {
Parent parentAgg = searchResponse.getAggregations().get("to_article");
assertEquals(parentAgg.getDocCount(), 1);
}
}
22 changes: 22 additions & 0 deletions ...arent-join/src/internalClusterTest/java/org/opensearch/join/query/ChildQuerySearchIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

package org.opensearch.join.query;

import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
import org.apache.lucene.search.join.ScoreMode;
import org.opensearch.action.explain.ExplainResponse;
import org.opensearch.action.index.IndexRequestBuilder;
Expand All @@ -42,6 +43,7 @@
import org.opensearch.common.lucene.search.function.FunctionScoreQuery;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.unit.TimeValue;
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.index.query.BoolQueryBuilder;
import org.opensearch.index.query.IdsQueryBuilder;
Expand All @@ -65,6 +67,8 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand All @@ -87,6 +91,7 @@
import static org.opensearch.join.query.JoinQueryBuilders.hasChildQuery;
import static org.opensearch.join.query.JoinQueryBuilders.hasParentQuery;
import static org.opensearch.join.query.JoinQueryBuilders.parentId;
import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertNoFailures;
Expand All @@ -100,6 +105,23 @@

public class ChildQuerySearchIT extends ParentChildTestCase {

public ChildQuerySearchIT(Settings settings) {
super(settings);
}

@ParametersFactory
public static Collection<Object[]> 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();
}

public void testMultiLevelChild() throws Exception {
assertAcked(
prepareCreate("test").setMapping(
Expand Down
22 changes: 22 additions & 0 deletions modules/parent-join/src/internalClusterTest/java/org/opensearch/join/query/InnerHitsIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,14 @@

package org.opensearch.join.query;

import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
import org.apache.lucene.search.join.ScoreMode;
import org.apache.lucene.util.ArrayUtil;
import org.opensearch.action.index.IndexRequestBuilder;
import org.opensearch.action.search.SearchPhaseExecutionException;
import org.opensearch.action.search.SearchResponse;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.index.IndexSettings;
import org.opensearch.index.query.BoolQueryBuilder;
import org.opensearch.index.query.InnerHitBuilder;
Expand All @@ -54,6 +57,7 @@
import org.opensearch.search.sort.SortOrder;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
Expand All @@ -73,6 +77,7 @@
import static org.opensearch.index.seqno.SequenceNumbers.UNASSIGNED_SEQ_NO;
import static org.opensearch.join.query.JoinQueryBuilders.hasChildQuery;
import static org.opensearch.join.query.JoinQueryBuilders.hasParentQuery;
import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount;
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertNoFailures;
Expand All @@ -87,6 +92,23 @@

public class InnerHitsIT extends ParentChildTestCase {

public InnerHitsIT(Settings settings) {
super(settings);
}

@ParametersFactory
public static Collection<Object[]> 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 Collection<Class<? extends Plugin>> nodePlugins() {
ArrayList<Class<? extends Plugin>> plugins = new ArrayList<>(super.nodePlugins());
Expand Down
7 changes: 6 additions & 1 deletion ...rent-join/src/internalClusterTest/java/org/opensearch/join/query/ParentChildTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.opensearch.plugins.Plugin;
import org.opensearch.test.InternalSettingsPlugin;
import org.opensearch.test.OpenSearchIntegTestCase;
import org.opensearch.test.ParameterizedOpenSearchIntegTestCase;

import java.io.IOException;
import java.util.Arrays;
Expand All @@ -50,7 +51,11 @@
import java.util.Map;

@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.SUITE)
public abstract class ParentChildTestCase extends OpenSearchIntegTestCase {
public abstract class ParentChildTestCase extends ParameterizedOpenSearchIntegTestCase {

public ParentChildTestCase(Settings dynamicSettings) {
super(dynamicSettings);
}

@Override
protected boolean ignoreExternalCluster() {
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/opensearch/common/settings/ClusterSettings.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,8 @@ public void apply(Settings value, Settings current, Settings previous) {
List.of(FeatureFlags.CONCURRENT_SEGMENT_SEARCH),
List.of(
SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING,
SearchService.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING
SearchService.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING,
SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_DISABLED_AGGS
),
List.of(FeatureFlags.TELEMETRY),
List.of(TelemetrySettings.TRACER_ENABLED_SETTING)
Expand Down
Loading

0 comments on commit 2f2cfd7

Please sign in to comment.