From c99ba635c8a7652cc6618b71e15ddde335008b8b Mon Sep 17 00:00:00 2001 From: Ticheng Lin <51488860+ticheng-aws@users.noreply.github.com> Date: Thu, 7 Sep 2023 20:08:51 -0700 Subject: [PATCH 1/4] Mute the query profile IT with concurrent execution (#9840) Signed-off-by: Ticheng Lin --- CHANGELOG.md | 1 + .../search/profile/query/QueryProfilerIT.java | 74 +------------------ 2 files changed, 3 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eabea882750f0..076eebb65f19c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -187,6 +187,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Cleanup Unreferenced file on segment merge failure ([#9503](https://github.com/opensearch-project/OpenSearch/pull/9503)) - Move ZStd to a plugin ([#9658](https://github.com/opensearch-project/OpenSearch/pull/9658)) - [Remote Store] Add support for Remote Translog Store upload stats in `_nodes/stats/` API ([#8908](https://github.com/opensearch-project/OpenSearch/pull/8908)) +- Mute the query profile IT with concurrent execution ([#9840](https://github.com/opensearch-project/OpenSearch/pull/9840)) ### Deprecated diff --git a/server/src/internalClusterTest/java/org/opensearch/search/profile/query/QueryProfilerIT.java b/server/src/internalClusterTest/java/org/opensearch/search/profile/query/QueryProfilerIT.java index 24f2b1bbdabfc..5f794d2abf878 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/profile/query/QueryProfilerIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/profile/query/QueryProfilerIT.java @@ -32,8 +32,6 @@ package org.opensearch.search.profile.query; -import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; - import org.apache.lucene.tests.util.English; import org.opensearch.action.index.IndexRequestBuilder; import org.opensearch.action.search.MultiSearchResponse; @@ -42,23 +40,20 @@ import org.opensearch.action.search.SearchType; import org.opensearch.action.search.ShardSearchFailure; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.query.QueryBuilder; import org.opensearch.index.query.QueryBuilders; import org.opensearch.search.SearchHit; import org.opensearch.search.profile.ProfileResult; import org.opensearch.search.profile.ProfileShardResult; import org.opensearch.search.sort.SortOrder; -import org.opensearch.test.ParameterizedOpenSearchIntegTestCase; +import org.opensearch.test.OpenSearchIntegTestCase; import java.util.Arrays; -import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; -import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING; import static org.opensearch.search.profile.query.RandomQueryGenerator.randomQueryBuilder; import static org.hamcrest.Matchers.emptyOrNullString; import static org.hamcrest.Matchers.equalTo; @@ -66,32 +61,8 @@ import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.nullValue; - -public class QueryProfilerIT extends ParameterizedOpenSearchIntegTestCase { - private final boolean concurrentSearchEnabled; - private static final String MAX_PREFIX = "max_"; - private static final String MIN_PREFIX = "min_"; - private static final String AVG_PREFIX = "avg_"; - private static final String TIMING_TYPE_COUNT_SUFFIX = "_count"; - - public QueryProfilerIT(Settings settings, boolean concurrentSearchEnabled) { - super(settings); - this.concurrentSearchEnabled = concurrentSearchEnabled; - } - - @ParametersFactory - public static Collection parameters() { - return Arrays.asList( - new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), false).build(), false }, - new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).build(), true } - ); - } - @Override - protected Settings featureFlagSettings() { - return Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.CONCURRENT_SEGMENT_SEARCH, "true").build(); - } +public class QueryProfilerIT extends OpenSearchIntegTestCase { /** * This test simply checks to make sure nothing crashes. Test indexes 100-150 documents, @@ -258,7 +229,6 @@ public void testSimpleMatch() throws Exception { assertEquals(result.getLuceneDescription(), "field1:one"); assertThat(result.getTime(), greaterThan(0L)); assertNotNull(result.getTimeBreakdown()); - assertQueryProfileResult(result); } CollectorResult result = searchProfiles.getCollectorResult(); @@ -301,7 +271,6 @@ public void testBool() throws Exception { assertThat(result.getTime(), greaterThan(0L)); assertNotNull(result.getTimeBreakdown()); assertEquals(result.getProfiledChildren().size(), 2); - assertQueryProfileResult(result); // Check the children List children = result.getProfiledChildren(); @@ -313,14 +282,12 @@ public void testBool() throws Exception { assertThat(childProfile.getTime(), greaterThan(0L)); assertNotNull(childProfile.getTimeBreakdown()); assertEquals(childProfile.getProfiledChildren().size(), 0); - assertQueryProfileResult(childProfile); childProfile = children.get(1); assertEquals(childProfile.getQueryName(), "TermQuery"); assertEquals(childProfile.getLuceneDescription(), "field1:two"); assertThat(childProfile.getTime(), greaterThan(0L)); assertNotNull(childProfile.getTimeBreakdown()); - assertQueryProfileResult(childProfile); } CollectorResult result = searchProfiles.getCollectorResult(); @@ -363,7 +330,6 @@ public void testEmptyBool() throws Exception { assertNotNull(result.getLuceneDescription()); assertThat(result.getTime(), greaterThan(0L)); assertNotNull(result.getTimeBreakdown()); - assertQueryProfileResult(result); } CollectorResult result = searchProfiles.getCollectorResult(); @@ -409,7 +375,6 @@ public void testCollapsingBool() throws Exception { assertNotNull(result.getLuceneDescription()); assertThat(result.getTime(), greaterThan(0L)); assertNotNull(result.getTimeBreakdown()); - assertQueryProfileResult(result); } CollectorResult result = searchProfiles.getCollectorResult(); @@ -450,7 +415,6 @@ public void testBoosting() throws Exception { assertNotNull(result.getLuceneDescription()); assertThat(result.getTime(), greaterThan(0L)); assertNotNull(result.getTimeBreakdown()); - assertQueryProfileResult(result); } CollectorResult result = searchProfiles.getCollectorResult(); @@ -491,7 +455,6 @@ public void testDisMaxRange() throws Exception { assertNotNull(result.getLuceneDescription()); assertThat(result.getTime(), greaterThan(0L)); assertNotNull(result.getTimeBreakdown()); - assertQueryProfileResult(result); } CollectorResult result = searchProfiles.getCollectorResult(); @@ -531,7 +494,6 @@ public void testRange() throws Exception { assertNotNull(result.getLuceneDescription()); assertThat(result.getTime(), greaterThan(0L)); assertNotNull(result.getTimeBreakdown()); - assertQueryProfileResult(result); } CollectorResult result = searchProfiles.getCollectorResult(); @@ -585,7 +547,6 @@ public void testPhrase() throws Exception { assertNotNull(result.getLuceneDescription()); assertThat(result.getTime(), greaterThan(0L)); assertNotNull(result.getTimeBreakdown()); - assertQueryProfileResult(result); } CollectorResult result = searchProfiles.getCollectorResult(); @@ -618,35 +579,4 @@ public void testNoProfile() throws Exception { assertThat("Profile response element should be an empty map", resp.getProfileResults().size(), equalTo(0)); } - private void assertQueryProfileResult(ProfileResult result) { - Map breakdown = result.getTimeBreakdown(); - Long maxSliceTime = result.getMaxSliceTime(); - Long minSliceTime = result.getMinSliceTime(); - Long avgSliceTime = result.getAvgSliceTime(); - if (concurrentSearchEnabled) { - assertNotNull(maxSliceTime); - assertNotNull(minSliceTime); - assertNotNull(avgSliceTime); - assertThat(breakdown.size(), equalTo(66)); - for (QueryTimingType queryTimingType : QueryTimingType.values()) { - if (queryTimingType != QueryTimingType.CREATE_WEIGHT) { - String maxTimingType = MAX_PREFIX + queryTimingType; - String minTimingType = MIN_PREFIX + queryTimingType; - String avgTimingType = AVG_PREFIX + queryTimingType; - assertNotNull(breakdown.get(maxTimingType)); - assertNotNull(breakdown.get(minTimingType)); - assertNotNull(breakdown.get(avgTimingType)); - assertNotNull(breakdown.get(maxTimingType + TIMING_TYPE_COUNT_SUFFIX)); - assertNotNull(breakdown.get(minTimingType + TIMING_TYPE_COUNT_SUFFIX)); - assertNotNull(breakdown.get(avgTimingType + TIMING_TYPE_COUNT_SUFFIX)); - } - } - } else { - assertThat(maxSliceTime, is(nullValue())); - assertThat(minSliceTime, is(nullValue())); - assertThat(avgSliceTime, is(nullValue())); - assertThat(breakdown.size(), equalTo(27)); - } - } - } From 967ef31f339b00722ee7467dc387c0a8fb16f462 Mon Sep 17 00:00:00 2001 From: Varun Bansal Date: Fri, 8 Sep 2023 11:47:52 +0530 Subject: [PATCH 2/4] Disable remote integrity check for encrypted repos for ChecksumBlobstoreFormat (#9908) Signed-off-by: bansvaru --- .../repositories/blobstore/ChecksumBlobStoreFormat.java | 2 +- .../gateway/remote/RemoteClusterStateServiceTests.java | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java b/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java index 744e2fbd1bfc7..7e1960171043a 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/ChecksumBlobStoreFormat.java @@ -222,7 +222,7 @@ public void writeAsync( WritePriority.HIGH, (size, position) -> new OffsetRangeIndexInputStream(input, size, position), expectedChecksum, - true + ((AsyncMultiStreamBlobContainer) blobContainer).remoteIntegrityCheckSupported() ) ) { ((AsyncMultiStreamBlobContainer) blobContainer).asyncBlobUpload(remoteTransferContainer.createWriteContext(), listener); diff --git a/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java b/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java index fe8dc0b564cda..9f5067420aab1 100644 --- a/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java +++ b/server/src/test/java/org/opensearch/gateway/remote/RemoteClusterStateServiceTests.java @@ -244,7 +244,11 @@ public void testWriteFullMetadataInParallelSuccess() throws IOException { assertEquals(writtenIndexMetadata.getIndex().getName(), "test-index"); assertEquals(writtenIndexMetadata.getIndex().getUUID(), "index-uuid"); long expectedChecksum = RemoteTransferContainer.checksumOfChecksum(new ByteArrayIndexInput("metadata-filename", writtenBytes), 8); - assertEquals(capturedWriteContext.getExpectedChecksum().longValue(), expectedChecksum); + if (capturedWriteContext.doRemoteDataIntegrityCheck()) { + assertEquals(capturedWriteContext.getExpectedChecksum().longValue(), expectedChecksum); + } else { + assertEquals(capturedWriteContext.getExpectedChecksum(), null); + } } From 7abfc17a6d1334a885e16d07aa3f6d1c875279c8 Mon Sep 17 00:00:00 2001 From: Ketan Verma <9292653+ketanv3@users.noreply.github.com> Date: Fri, 8 Sep 2023 17:16:41 +0530 Subject: [PATCH 3/4] Improve performance of rounding dates in date_histogram aggregation (#9727) * Improve performance of rounding dates in date_histogram aggregation Signed-off-by: Ketan Verma * Minor refactoring changes Signed-off-by: Ketan Verma --------- Signed-off-by: Ketan Verma --- CHANGELOG.md | 1 + .../common/ArrayRoundingBenchmark.java | 147 ++++++++++++++++++ .../java/org/opensearch/common/Rounding.java | 100 ++++++++++-- .../org/opensearch/common/RoundingTests.java | 22 +++ 4 files changed, 261 insertions(+), 9 deletions(-) create mode 100644 benchmarks/src/main/java/org/opensearch/common/ArrayRoundingBenchmark.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 076eebb65f19c..ce67273f2a6f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -182,6 +182,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Redefine telemetry context restoration and propagation ([#9617](https://github.com/opensearch-project/OpenSearch/pull/9617)) - Use non-concurrent path for sort request on timeseries index and field([#9562](https://github.com/opensearch-project/OpenSearch/pull/9562)) - Added sampler based on `Blanket Probabilistic Sampling rate` and `Override for on demand` ([#9621](https://github.com/opensearch-project/OpenSearch/issues/9621)) +- Improve performance of rounding dates in date_histogram aggregation ([#9727](https://github.com/opensearch-project/OpenSearch/pull/9727)) - [Remote Store] Add support for Remote Translog Store stats in `_remotestore/stats/` API ([#9263](https://github.com/opensearch-project/OpenSearch/pull/9263)) - Add support for query profiler with concurrent aggregation ([#9248](https://github.com/opensearch-project/OpenSearch/pull/9248)) - Cleanup Unreferenced file on segment merge failure ([#9503](https://github.com/opensearch-project/OpenSearch/pull/9503)) diff --git a/benchmarks/src/main/java/org/opensearch/common/ArrayRoundingBenchmark.java b/benchmarks/src/main/java/org/opensearch/common/ArrayRoundingBenchmark.java new file mode 100644 index 0000000000000..64c0a9e1d7aa6 --- /dev/null +++ b/benchmarks/src/main/java/org/opensearch/common/ArrayRoundingBenchmark.java @@ -0,0 +1,147 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; + +import java.util.Random; +import java.util.function.Supplier; + +@Fork(value = 3) +@Warmup(iterations = 3, time = 1) +@Measurement(iterations = 1, time = 1) +@BenchmarkMode(Mode.Throughput) +public class ArrayRoundingBenchmark { + + @Benchmark + public void round(Blackhole bh, Options opts) { + Rounding.Prepared rounding = opts.supplier.get(); + for (long key : opts.queries) { + bh.consume(rounding.round(key)); + } + } + + @State(Scope.Benchmark) + public static class Options { + @Param({ + "1", + "2", + "3", + "4", + "5", + "6", + "7", + "8", + "9", + "10", + "12", + "14", + "16", + "18", + "20", + "22", + "24", + "26", + "29", + "32", + "37", + "41", + "45", + "49", + "54", + "60", + "64", + "74", + "83", + "90", + "98", + "108", + "118", + "128", + "144", + "159", + "171", + "187", + "204", + "229", + "256" }) + public Integer size; + + @Param({ "binary", "linear" }) + public String type; + + @Param({ "uniform", "skewed_edge", "skewed_center" }) + public String distribution; + + public long[] queries; + public Supplier supplier; + + @Setup + public void setup() { + Random random = new Random(size); + long[] values = new long[size]; + for (int i = 1; i < values.length; i++) { + values[i] = values[i - 1] + 100; + } + + long range = values[values.length - 1] - values[0] + 100; + long mean, stddev; + queries = new long[1000000]; + + switch (distribution) { + case "uniform": // all values equally likely. + for (int i = 0; i < queries.length; i++) { + queries[i] = values[0] + (nextPositiveLong(random) % range); + } + break; + case "skewed_edge": // distribution centered at p90 with ± 5% stddev. + mean = values[0] + (long) (range * 0.9); + stddev = (long) (range * 0.05); + for (int i = 0; i < queries.length; i++) { + queries[i] = Math.max(values[0], mean + (long) (random.nextGaussian() * stddev)); + } + break; + case "skewed_center": // distribution centered at p50 with ± 5% stddev. + mean = values[0] + (long) (range * 0.5); + stddev = (long) (range * 0.05); + for (int i = 0; i < queries.length; i++) { + queries[i] = Math.max(values[0], mean + (long) (random.nextGaussian() * stddev)); + } + break; + default: + throw new IllegalArgumentException("invalid distribution: " + distribution); + } + + switch (type) { + case "binary": + supplier = () -> new Rounding.BinarySearchArrayRounding(values, size, null); + break; + case "linear": + supplier = () -> new Rounding.BidirectionalLinearSearchArrayRounding(values, size, null); + break; + default: + throw new IllegalArgumentException("invalid type: " + type); + } + } + + private static long nextPositiveLong(Random random) { + return random.nextLong() & Long.MAX_VALUE; + } + } +} diff --git a/server/src/main/java/org/opensearch/common/Rounding.java b/server/src/main/java/org/opensearch/common/Rounding.java index 65ffdafc423fd..667eb4529fe38 100644 --- a/server/src/main/java/org/opensearch/common/Rounding.java +++ b/server/src/main/java/org/opensearch/common/Rounding.java @@ -37,6 +37,7 @@ import org.opensearch.OpenSearchException; import org.opensearch.common.LocalTimeOffset.Gap; import org.opensearch.common.LocalTimeOffset.Overlap; +import org.opensearch.common.annotation.InternalApi; import org.opensearch.common.time.DateUtils; import org.opensearch.common.unit.TimeValue; import org.opensearch.core.common.io.stream.StreamInput; @@ -412,6 +413,21 @@ public Rounding build() { } private abstract class PreparedRounding implements Prepared { + /** + * The maximum limit up to which array-based prepared rounding is used. + * 128 is a power of two that isn't huge. We might be able to do + * better if the limit was based on the actual type of prepared + * rounding but this'll do for now. + */ + private static final int DEFAULT_ARRAY_ROUNDING_MAX_THRESHOLD = 128; + + /** + * The maximum limit up to which linear search is used, otherwise binary search is used. + * This is because linear search is much faster on small arrays. + * Benchmark results: PR #9727 + */ + private static final int LINEAR_SEARCH_ARRAY_ROUNDING_MAX_THRESHOLD = 64; + /** * Attempt to build a {@link Prepared} implementation that relies on pre-calcuated * "round down" points. If there would be more than {@code max} points then return @@ -435,7 +451,9 @@ protected Prepared maybeUseArray(long minUtcMillis, long maxUtcMillis, int max) values = ArrayUtil.grow(values, i + 1); values[i++] = rounded; } - return new ArrayRounding(values, i, this); + return i <= LINEAR_SEARCH_ARRAY_ROUNDING_MAX_THRESHOLD + ? new BidirectionalLinearSearchArrayRounding(values, i, this) + : new BinarySearchArrayRounding(values, i, this); } } @@ -521,12 +539,11 @@ private LocalDateTime truncateLocalDateTime(LocalDateTime localDateTime) { @Override public Prepared prepare(long minUtcMillis, long maxUtcMillis) { - /* - * 128 is a power of two that isn't huge. We might be able to do - * better if the limit was based on the actual type of prepared - * rounding but this'll do for now. - */ - return prepareOffsetOrJavaTimeRounding(minUtcMillis, maxUtcMillis).maybeUseArray(minUtcMillis, maxUtcMillis, 128); + return prepareOffsetOrJavaTimeRounding(minUtcMillis, maxUtcMillis).maybeUseArray( + minUtcMillis, + maxUtcMillis, + PreparedRounding.DEFAULT_ARRAY_ROUNDING_MAX_THRESHOLD + ); } private TimeUnitPreparedRounding prepareOffsetOrJavaTimeRounding(long minUtcMillis, long maxUtcMillis) { @@ -1330,14 +1347,19 @@ public static Rounding read(StreamInput in) throws IOException { /** * Implementation of {@link Prepared} using pre-calculated "round down" points. * + *

+ * It uses binary search to find the greatest round-down point less than or equal to the given timestamp. + * * @opensearch.internal */ - private static class ArrayRounding implements Prepared { + @InternalApi + static class BinarySearchArrayRounding implements Prepared { private final long[] values; private final int max; private final Prepared delegate; - private ArrayRounding(long[] values, int max, Prepared delegate) { + BinarySearchArrayRounding(long[] values, int max, Prepared delegate) { + assert max > 0 : "at least one round-down point must be present"; this.values = values; this.max = max; this.delegate = delegate; @@ -1365,4 +1387,64 @@ public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { return delegate.roundingSize(utcMillis, timeUnit); } } + + /** + * Implementation of {@link Prepared} using pre-calculated "round down" points. + * + *

+ * It uses linear search to find the greatest round-down point less than or equal to the given timestamp. + * For small inputs (≤ 64 elements), this can be much faster than binary search as it avoids the penalty of + * branch mispredictions and pipeline stalls, and accesses memory sequentially. + * + *

+ * It uses "meet in the middle" linear search to avoid the worst case scenario when the desired element is present + * at either side of the array. This is helpful for time-series data where velocity increases over time, so more + * documents are likely to find a greater timestamp which is likely to be present on the right end of the array. + * + * @opensearch.internal + */ + @InternalApi + static class BidirectionalLinearSearchArrayRounding implements Prepared { + private final long[] ascending; + private final long[] descending; + private final Prepared delegate; + + BidirectionalLinearSearchArrayRounding(long[] values, int max, Prepared delegate) { + assert max > 0 : "at least one round-down point must be present"; + this.delegate = delegate; + int len = (max + 1) >>> 1; // rounded-up to handle odd number of values + ascending = new long[len]; + descending = new long[len]; + + for (int i = 0; i < len; i++) { + ascending[i] = values[i]; + descending[i] = values[max - i - 1]; + } + } + + @Override + public long round(long utcMillis) { + int i = 0; + for (; i < ascending.length; i++) { + if (descending[i] <= utcMillis) { + return descending[i]; + } + if (ascending[i] > utcMillis) { + assert i > 0 : "utcMillis must be after " + ascending[0]; + return ascending[i - 1]; + } + } + return ascending[i - 1]; + } + + @Override + public long nextRoundingValue(long utcMillis) { + return delegate.nextRoundingValue(utcMillis); + } + + @Override + public double roundingSize(long utcMillis, DateTimeUnit timeUnit) { + return delegate.roundingSize(utcMillis, timeUnit); + } + } } diff --git a/server/src/test/java/org/opensearch/common/RoundingTests.java b/server/src/test/java/org/opensearch/common/RoundingTests.java index e0c44e3516e7b..0ebfe02dc7641 100644 --- a/server/src/test/java/org/opensearch/common/RoundingTests.java +++ b/server/src/test/java/org/opensearch/common/RoundingTests.java @@ -1143,6 +1143,28 @@ public void testNonMillisecondsBasedUnitCalendarRoundingSize() { assertThat(prepared.roundingSize(thirdQuarter, Rounding.DateTimeUnit.HOUR_OF_DAY), closeTo(2208.0, 0.000001)); } + public void testArrayRoundingImplementations() { + int length = randomIntBetween(1, 256); + long[] values = new long[length]; + for (int i = 1; i < values.length; i++) { + values[i] = values[i - 1] + (randomNonNegativeLong() % 100); + } + + Rounding.Prepared binarySearchImpl = new Rounding.BinarySearchArrayRounding(values, length, null); + Rounding.Prepared linearSearchImpl = new Rounding.BidirectionalLinearSearchArrayRounding(values, length, null); + + for (int i = 0; i < 100000; i++) { + long key = values[0] + (randomNonNegativeLong() % (100 + values[length - 1] - values[0])); + assertEquals(binarySearchImpl.round(key), linearSearchImpl.round(key)); + } + + AssertionError exception = expectThrows(AssertionError.class, () -> { binarySearchImpl.round(values[0] - 1); }); + assertEquals("utcMillis must be after " + values[0], exception.getMessage()); + + exception = expectThrows(AssertionError.class, () -> { linearSearchImpl.round(values[0] - 1); }); + assertEquals("utcMillis must be after " + values[0], exception.getMessage()); + } + private void assertInterval(long rounded, long nextRoundingValue, Rounding rounding, int minutes, ZoneId tz) { assertInterval(rounded, dateBetween(rounded, nextRoundingValue), nextRoundingValue, rounding, tz); long millisPerMinute = 60_000; From 8e5e54b3a0d4e9c49f6ff062af361cf88ec76322 Mon Sep 17 00:00:00 2001 From: Neetika Singhal Date: Fri, 8 Sep 2023 06:16:27 -0700 Subject: [PATCH 4/4] Allow parameterization of tests with OpenSearchIntegTestCase.SuiteScopeTestCase annotation (#9916) Signed-off-by: Neetika Singhal Signed-off-by: Andriy Redko Co-authored-by: Andriy Redko --- CHANGELOG.md | 1 + .../search/aggregations/MissingValueIT.java | 28 ++++++++++++++++++- .../test/OpenSearchIntegTestCase.java | 18 ++++++++++-- 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce67273f2a6f8..d6b4660d85d49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -188,6 +188,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Cleanup Unreferenced file on segment merge failure ([#9503](https://github.com/opensearch-project/OpenSearch/pull/9503)) - Move ZStd to a plugin ([#9658](https://github.com/opensearch-project/OpenSearch/pull/9658)) - [Remote Store] Add support for Remote Translog Store upload stats in `_nodes/stats/` API ([#8908](https://github.com/opensearch-project/OpenSearch/pull/8908)) +- Allow parameterization of tests with OpenSearchIntegTestCase.SuiteScopeTestCase annotation ([#9916](https://github.com/opensearch-project/OpenSearch/pull/9916)) - Mute the query profile IT with concurrent execution ([#9840](https://github.com/opensearch-project/OpenSearch/pull/9840)) ### Deprecated diff --git a/server/src/internalClusterTest/java/org/opensearch/search/aggregations/MissingValueIT.java b/server/src/internalClusterTest/java/org/opensearch/search/aggregations/MissingValueIT.java index 26bfe59618275..e6325987d330f 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/aggregations/MissingValueIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/aggregations/MissingValueIT.java @@ -32,8 +32,12 @@ package org.opensearch.search.aggregations; +import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; + import org.opensearch.action.search.SearchResponse; import org.opensearch.common.geo.GeoPoint; +import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.search.aggregations.bucket.histogram.DateHistogramInterval; import org.opensearch.search.aggregations.bucket.histogram.Histogram; import org.opensearch.search.aggregations.bucket.terms.Terms; @@ -43,7 +47,12 @@ import org.opensearch.search.aggregations.metrics.Percentiles; import org.opensearch.search.aggregations.metrics.Stats; import org.opensearch.test.OpenSearchIntegTestCase; +import org.opensearch.test.ParameterizedOpenSearchIntegTestCase; + +import java.util.Arrays; +import java.util.Collection; +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.dateHistogram; import static org.opensearch.search.aggregations.AggregationBuilders.geoCentroid; @@ -56,7 +65,24 @@ import static org.hamcrest.Matchers.closeTo; @OpenSearchIntegTestCase.SuiteScopeTestCase -public class MissingValueIT extends OpenSearchIntegTestCase { +public class MissingValueIT extends ParameterizedOpenSearchIntegTestCase { + + public MissingValueIT(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 int maximumNumberOfShards() { 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 f06a6e26defeb..65e5a18c89949 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java @@ -2316,8 +2316,12 @@ private static void initializeSuiteScope() throws Exception { */ assert INSTANCE == null; if (isSuiteScopedTest(targetClass)) { - // note we need to do this this way to make sure this is reproducible - INSTANCE = (OpenSearchIntegTestCase) targetClass.getConstructor().newInstance(); + // note we need to do this way to make sure this is reproducible + if (isSuiteScopedTestParameterized(targetClass)) { + INSTANCE = (OpenSearchIntegTestCase) targetClass.getConstructor(Settings.class).newInstance(Settings.EMPTY); + } else { + INSTANCE = (OpenSearchIntegTestCase) targetClass.getConstructor().newInstance(); + } boolean success = false; try { INSTANCE.printTestMessage("setup"); @@ -2412,6 +2416,16 @@ private static boolean isSuiteScopedTest(Class clazz) { return clazz.getAnnotation(SuiteScopeTestCase.class) != null; } + /* + * For tests defined with, SuiteScopeTestCase return true if the + * class has a constructor that takes a single Settings parameter + * */ + private static boolean isSuiteScopedTestParameterized(Class clazz) { + return Arrays.stream(clazz.getConstructors()) + .filter(x -> x.getParameterTypes().length == 1) + .anyMatch(x -> x.getParameterTypes()[0].equals(Settings.class)); + } + /** * If a test is annotated with {@link SuiteScopeTestCase} * the checks and modifications that are applied to the used test cluster are only done after all tests