From 08a4f6d801c9043aa370017a67810d9f0b140204 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 9 Oct 2023 09:59:41 -0700 Subject: [PATCH 1/6] Adds and tests took time for QuerySearchResult Signed-off-by: Peter Alfonsi --- .../opensearch/search/query/QueryPhase.java | 4 +- .../search/query/QuerySearchResult.java | 11 ++ .../search/query/QueryPhaseTests.java | 116 ++++++++++++++++++ .../search/query/QuerySearchResultTests.java | 2 + 4 files changed, 132 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/search/query/QueryPhase.java b/server/src/main/java/org/opensearch/search/query/QueryPhase.java index f3cf2c13ecdef..14b239d3270d4 100644 --- a/server/src/main/java/org/opensearch/search/query/QueryPhase.java +++ b/server/src/main/java/org/opensearch/search/query/QueryPhase.java @@ -131,6 +131,7 @@ public void preProcess(SearchContext context) { } public void execute(SearchContext searchContext) throws QueryPhaseExecutionException { + long startTime = System.nanoTime(); if (searchContext.hasOnlySuggest()) { suggestProcessor.process(searchContext); searchContext.queryResult() @@ -138,6 +139,7 @@ public void execute(SearchContext searchContext) throws QueryPhaseExecutionExcep new TopDocsAndMaxScore(new TopDocs(new TotalHits(0, TotalHits.Relation.EQUAL_TO), Lucene.EMPTY_SCORE_DOCS), Float.NaN), new DocValueFormat[0] ); + searchContext.queryResult().setTookTimeNanos(System.nanoTime() - startTime); return; } @@ -165,6 +167,7 @@ public void execute(SearchContext searchContext) throws QueryPhaseExecutionExcep ); searchContext.queryResult().profileResults(shardResults); } + searchContext.queryResult().setTookTimeNanos(System.nanoTime() - startTime); } // making public for testing @@ -292,7 +295,6 @@ static boolean executeInternal(SearchContext searchContext, QueryPhaseSearcher q queryResult.nodeQueueSize(rExecutor.getCurrentQueueSize()); queryResult.serviceTimeEWMA((long) rExecutor.getTaskExecutionEWMA()); } - return shouldRescore; } finally { // Search phase has finished, no longer need to check for timeout diff --git a/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java b/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java index 7de605a244d09..5a8cf06bd8b0a 100644 --- a/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java +++ b/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java @@ -87,6 +87,7 @@ public final class QuerySearchResult extends SearchPhaseResult { private int nodeQueueSize = -1; private final boolean isNull; + private long tookTimeNanos; public QuerySearchResult() { this(false); @@ -364,6 +365,7 @@ public void readFromWithId(ShardSearchContextId id, StreamInput in) throws IOExc nodeQueueSize = in.readInt(); setShardSearchRequest(in.readOptionalWriteable(ShardSearchRequest::new)); setRescoreDocIds(new RescoreDocIds(in)); + tookTimeNanos = in.readVLong(); } @Override @@ -406,6 +408,7 @@ public void writeToNoId(StreamOutput out) throws IOException { out.writeInt(nodeQueueSize); out.writeOptionalWriteable(getShardSearchRequest()); getRescoreDocIds().writeTo(out); + out.writeVLong(tookTimeNanos); // VLong as took time should always be positive } public TotalHits getTotalHits() { @@ -415,4 +418,12 @@ public TotalHits getTotalHits() { public float getMaxScore() { return maxScore; } + + public long getTookTimeNanos() { + return tookTimeNanos; + } + + public void setTookTimeNanos(long tookTime) { + tookTimeNanos = tookTime; + } } diff --git a/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java b/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java index 39126a607f968..93c54f93801ab 100644 --- a/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java +++ b/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java @@ -85,9 +85,14 @@ import org.apache.lucene.tests.index.RandomIndexWriter; import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.FixedBitSet; +import org.opensearch.action.OriginalIndices; +import org.opensearch.action.search.SearchRequest; import org.opensearch.action.search.SearchShardTask; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; +import org.opensearch.core.common.Strings; +import org.opensearch.core.index.Index; +import org.opensearch.core.index.shard.ShardId; import org.opensearch.core.tasks.TaskCancelledException; import org.opensearch.index.mapper.DateFieldMapper; import org.opensearch.index.mapper.MappedFieldType; @@ -103,9 +108,11 @@ import org.opensearch.lucene.queries.MinDocQuery; import org.opensearch.search.DocValueFormat; import org.opensearch.search.collapse.CollapseBuilder; +import org.opensearch.search.internal.AliasFilter; import org.opensearch.search.internal.ContextIndexSearcher; import org.opensearch.search.internal.ScrollContext; import org.opensearch.search.internal.SearchContext; +import org.opensearch.search.internal.ShardSearchRequest; import org.opensearch.search.sort.SortAndFormats; import org.opensearch.test.TestSearchContext; import org.opensearch.threadpool.ThreadPool; @@ -115,6 +122,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.LinkedList; import java.util.List; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -1145,6 +1153,114 @@ public void testQueryTimeoutChecker() throws Exception { createTimeoutCheckerThenWaitThenRun(timeCacheLifespan / 4, timeCacheLifespan / 2 + timeTolerance, false, true); } + public void testQuerySearchResultTookTime() throws IOException { + int sleepMillis = randomIntBetween(500, 4000); // between 0.5 and 4 sec + DelayedQueryPhaseSearcher delayedQueryPhaseSearcher = new DelayedQueryPhaseSearcher(sleepMillis); + + // we need to test queryPhase.execute(), not executeInternal(), since that's what the timer wraps around + // for that we must set up a searchContext with more functionality than the TestSearchContext, + // which requires a bit of complexity with test classes + + Directory dir = newDirectory(); + final Sort sort = new Sort(new SortField("rank", SortField.Type.INT)); + IndexWriterConfig iwc = newIndexWriterConfig().setIndexSort(sort); + RandomIndexWriter w = new RandomIndexWriter(random(), dir, iwc); + Document doc = new Document(); + for (int i = 0; i < 10; i++) { + doc.add(new StringField("foo", Integer.toString(i), Store.NO)); + } + w.addDocument(doc); + w.close(); + IndexReader reader = DirectoryReader.open(dir); + + QueryShardContext queryShardContext = mock(QueryShardContext.class); + when(queryShardContext.fieldMapper("user")).thenReturn( + new NumberFieldType("user", NumberType.INTEGER, true, false, true, false, null, Collections.emptyMap()) + ); + + Index index = new Index("IndexName", "UUID"); + ShardId shardId = new ShardId(index, 0); + long nowInMillis = System.currentTimeMillis(); + String clusterAlias = randomBoolean() ? null : randomAlphaOfLengthBetween(3, 10); + SearchRequest searchRequest = new SearchRequest(); + searchRequest.allowPartialSearchResults(randomBoolean()); + ShardSearchRequest request = new ShardSearchRequest( + OriginalIndices.NONE, + searchRequest, + shardId, + 1, + AliasFilter.EMPTY, + 1f, + nowInMillis, + clusterAlias, + Strings.EMPTY_ARRAY + ); + TestSearchContextWithRequest searchContext = new TestSearchContextWithRequest( + queryShardContext, + indexShard, + newEarlyTerminationContextSearcher(reader, 0, executor), + request + ); + + QueryPhase queryPhase = new QueryPhase(delayedQueryPhaseSearcher); + queryPhase.execute(searchContext); + long tookTime = searchContext.queryResult().getTookTimeNanos(); + assertTrue(tookTime >= (long) sleepMillis * 1000000); + reader.close(); + dir.close(); + } + + private class TestSearchContextWithRequest extends TestSearchContext { + ShardSearchRequest request; + Query query; + + public TestSearchContextWithRequest( + QueryShardContext queryShardContext, + IndexShard indexShard, + ContextIndexSearcher searcher, + ShardSearchRequest request + ) { + super(queryShardContext, indexShard, searcher); + this.request = request; + this.query = new TermQuery(new Term("foo", "bar")); + } + + @Override + public ShardSearchRequest request() { + return request; + } + + @Override + public Query query() { + return this.query; + } + } + + private class DelayedQueryPhaseSearcher extends QueryPhase.DefaultQueryPhaseSearcher implements QueryPhaseSearcher { + // add delay into searchWith + private final int sleepMillis; + + public DelayedQueryPhaseSearcher(int sleepMillis) { + super(); + this.sleepMillis = sleepMillis; + } + + @Override + public boolean searchWith( + SearchContext searchContext, + ContextIndexSearcher searcher, + Query query, + LinkedList collectors, + boolean hasFilterCollector, + boolean hasTimeout + ) throws IOException { + try { + Thread.sleep(sleepMillis); + } catch (Exception ignored) {} + return super.searchWith(searchContext, searcher, query, collectors, hasFilterCollector, hasTimeout); + } + } + private void createTimeoutCheckerThenWaitThenRun( long timeout, long sleepAfterCreation, diff --git a/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java b/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java index 41e4e1ae45a73..0d83777dbd4cf 100644 --- a/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java +++ b/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java @@ -99,6 +99,7 @@ private static QuerySearchResult createTestInstance() throws Exception { if (randomBoolean()) { result.aggregations(InternalAggregationsTests.createTestInstance()); } + assertEquals(0, result.getTookTimeNanos()); return result; } @@ -118,6 +119,7 @@ public void testSerialization() throws Exception { assertEquals(aggs.asList(), deserializedAggs.asList()); } assertEquals(querySearchResult.terminatedEarly(), deserialized.terminatedEarly()); + assertEquals(querySearchResult.getTookTimeNanos(), deserialized.getTookTimeNanos()); } public void testNullResponse() throws Exception { From dfd9128f331bafb58a264e3c4f5bfc6c8fa2f0c5 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 18 Oct 2023 17:48:44 -0700 Subject: [PATCH 2/6] Addressed Ankit's comments Signed-off-by: Peter Alfonsi --- .../org/opensearch/search/query/QueryPhase.java | 2 +- .../opensearch/search/query/QuerySearchResult.java | 14 +++++++++++--- .../opensearch/search/query/QueryPhaseTests.java | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/query/QueryPhase.java b/server/src/main/java/org/opensearch/search/query/QueryPhase.java index 14b239d3270d4..e6c281fdf74f4 100644 --- a/server/src/main/java/org/opensearch/search/query/QueryPhase.java +++ b/server/src/main/java/org/opensearch/search/query/QueryPhase.java @@ -131,7 +131,7 @@ public void preProcess(SearchContext context) { } public void execute(SearchContext searchContext) throws QueryPhaseExecutionException { - long startTime = System.nanoTime(); + final long startTime = System.nanoTime(); if (searchContext.hasOnlySuggest()) { suggestProcessor.process(searchContext); searchContext.queryResult() diff --git a/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java b/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java index 5a8cf06bd8b0a..bb484214325dd 100644 --- a/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java +++ b/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java @@ -34,6 +34,7 @@ import org.apache.lucene.search.FieldDoc; import org.apache.lucene.search.TotalHits; +import org.opensearch.Version; import org.opensearch.common.io.stream.DelayableWriteable; import org.opensearch.common.lucene.search.TopDocsAndMaxScore; import org.opensearch.core.common.io.stream.StreamInput; @@ -88,6 +89,7 @@ public final class QuerySearchResult extends SearchPhaseResult { private final boolean isNull; private long tookTimeNanos; + private final static Version minimumTookTimeVersion = Version.V_3_0_0; public QuerySearchResult() { this(false); @@ -365,7 +367,11 @@ public void readFromWithId(ShardSearchContextId id, StreamInput in) throws IOExc nodeQueueSize = in.readInt(); setShardSearchRequest(in.readOptionalWriteable(ShardSearchRequest::new)); setRescoreDocIds(new RescoreDocIds(in)); - tookTimeNanos = in.readVLong(); + if (in.getVersion().onOrAfter(minimumTookTimeVersion)) { + tookTimeNanos = in.readVLong(); + } else { + tookTimeNanos = -1L; + } } @Override @@ -408,7 +414,9 @@ public void writeToNoId(StreamOutput out) throws IOException { out.writeInt(nodeQueueSize); out.writeOptionalWriteable(getShardSearchRequest()); getRescoreDocIds().writeTo(out); - out.writeVLong(tookTimeNanos); // VLong as took time should always be positive + if (out.getVersion().onOrAfter(minimumTookTimeVersion)) { + out.writeVLong(tookTimeNanos); // VLong as took time should always be positive + } } public TotalHits getTotalHits() { @@ -423,7 +431,7 @@ public long getTookTimeNanos() { return tookTimeNanos; } - public void setTookTimeNanos(long tookTime) { + void setTookTimeNanos(long tookTime) { tookTimeNanos = tookTime; } } diff --git a/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java b/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java index 93c54f93801ab..7aa37a4cbe342 100644 --- a/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java +++ b/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java @@ -1154,7 +1154,7 @@ public void testQueryTimeoutChecker() throws Exception { } public void testQuerySearchResultTookTime() throws IOException { - int sleepMillis = randomIntBetween(500, 4000); // between 0.5 and 4 sec + int sleepMillis = randomIntBetween(10, 100); // between 0.01 and 0.1 sec DelayedQueryPhaseSearcher delayedQueryPhaseSearcher = new DelayedQueryPhaseSearcher(sleepMillis); // we need to test queryPhase.execute(), not executeInternal(), since that's what the timer wraps around From a9ab3278c9c8cb748c1fc0ab4a285832a0faab98 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 19 Oct 2023 21:36:39 -0700 Subject: [PATCH 3/6] Removed minimum version variable Signed-off-by: Peter Alfonsi --- .../java/org/opensearch/search/query/QuerySearchResult.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java b/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java index bb484214325dd..d3bfbea847169 100644 --- a/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java +++ b/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java @@ -89,7 +89,6 @@ public final class QuerySearchResult extends SearchPhaseResult { private final boolean isNull; private long tookTimeNanos; - private final static Version minimumTookTimeVersion = Version.V_3_0_0; public QuerySearchResult() { this(false); @@ -367,7 +366,7 @@ public void readFromWithId(ShardSearchContextId id, StreamInput in) throws IOExc nodeQueueSize = in.readInt(); setShardSearchRequest(in.readOptionalWriteable(ShardSearchRequest::new)); setRescoreDocIds(new RescoreDocIds(in)); - if (in.getVersion().onOrAfter(minimumTookTimeVersion)) { + if (in.getVersion().onOrAfter(Version.V_3_0_0)) { tookTimeNanos = in.readVLong(); } else { tookTimeNanos = -1L; @@ -414,7 +413,7 @@ public void writeToNoId(StreamOutput out) throws IOException { out.writeInt(nodeQueueSize); out.writeOptionalWriteable(getShardSearchRequest()); getRescoreDocIds().writeTo(out); - if (out.getVersion().onOrAfter(minimumTookTimeVersion)) { + if (out.getVersion().onOrAfter(Version.V_3_0_0)) { out.writeVLong(tookTimeNanos); // VLong as took time should always be positive } } From 4e57f4c03779160d54842a2db628e174ed583724 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 26 Oct 2023 12:07:27 -0700 Subject: [PATCH 4/6] Changed type for tookTime from long to Long, to address Sagar's comment Signed-off-by: Peter Alfonsi --- .../search/query/QuerySearchResult.java | 10 ++--- .../search/query/QueryPhaseTests.java | 2 +- .../search/query/QuerySearchResultTests.java | 40 +++++++++++-------- 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java b/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java index d3bfbea847169..2ad601f3bf4a6 100644 --- a/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java +++ b/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java @@ -88,7 +88,7 @@ public final class QuerySearchResult extends SearchPhaseResult { private int nodeQueueSize = -1; private final boolean isNull; - private long tookTimeNanos; + private Long tookTimeNanos = null; public QuerySearchResult() { this(false); @@ -367,9 +367,9 @@ public void readFromWithId(ShardSearchContextId id, StreamInput in) throws IOExc setShardSearchRequest(in.readOptionalWriteable(ShardSearchRequest::new)); setRescoreDocIds(new RescoreDocIds(in)); if (in.getVersion().onOrAfter(Version.V_3_0_0)) { - tookTimeNanos = in.readVLong(); + tookTimeNanos = in.readOptionalLong(); } else { - tookTimeNanos = -1L; + tookTimeNanos = null; } } @@ -414,7 +414,7 @@ public void writeToNoId(StreamOutput out) throws IOException { out.writeOptionalWriteable(getShardSearchRequest()); getRescoreDocIds().writeTo(out); if (out.getVersion().onOrAfter(Version.V_3_0_0)) { - out.writeVLong(tookTimeNanos); // VLong as took time should always be positive + out.writeOptionalLong(tookTimeNanos); } } @@ -426,7 +426,7 @@ public float getMaxScore() { return maxScore; } - public long getTookTimeNanos() { + public Long getTookTimeNanos() { return tookTimeNanos; } diff --git a/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java b/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java index 7aa37a4cbe342..ef30cea39be5c 100644 --- a/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java +++ b/server/src/test/java/org/opensearch/search/query/QueryPhaseTests.java @@ -1204,7 +1204,7 @@ public void testQuerySearchResultTookTime() throws IOException { QueryPhase queryPhase = new QueryPhase(delayedQueryPhaseSearcher); queryPhase.execute(searchContext); - long tookTime = searchContext.queryResult().getTookTimeNanos(); + Long tookTime = searchContext.queryResult().getTookTimeNanos(); assertTrue(tookTime >= (long) sleepMillis * 1000000); reader.close(); dir.close(); diff --git a/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java b/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java index 0d83777dbd4cf..4c7770c563764 100644 --- a/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java +++ b/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java @@ -99,27 +99,35 @@ private static QuerySearchResult createTestInstance() throws Exception { if (randomBoolean()) { result.aggregations(InternalAggregationsTests.createTestInstance()); } - assertEquals(0, result.getTookTimeNanos()); + assertNull(result.getTookTimeNanos()); return result; } public void testSerialization() throws Exception { - QuerySearchResult querySearchResult = createTestInstance(); - QuerySearchResult deserialized = copyWriteable(querySearchResult, namedWriteableRegistry, QuerySearchResult::new); - assertEquals(querySearchResult.getContextId().getId(), deserialized.getContextId().getId()); - assertNull(deserialized.getSearchShardTarget()); - assertEquals(querySearchResult.topDocs().maxScore, deserialized.topDocs().maxScore, 0f); - assertEquals(querySearchResult.topDocs().topDocs.totalHits, deserialized.topDocs().topDocs.totalHits); - assertEquals(querySearchResult.from(), deserialized.from()); - assertEquals(querySearchResult.size(), deserialized.size()); - assertEquals(querySearchResult.hasAggs(), deserialized.hasAggs()); - if (deserialized.hasAggs()) { - Aggregations aggs = querySearchResult.consumeAggs().expand(); - Aggregations deserializedAggs = deserialized.consumeAggs().expand(); - assertEquals(aggs.asList(), deserializedAggs.asList()); + for (int i = 0; i < 2; i++) { + QuerySearchResult querySearchResult = createTestInstance(); + if (i == 0) { + querySearchResult.setTookTimeNanos(1000L); // test both an initialized and an uninitialized null value + } + QuerySearchResult deserialized = copyWriteable(querySearchResult, namedWriteableRegistry, QuerySearchResult::new); + assertEquals(querySearchResult.getContextId().getId(), deserialized.getContextId().getId()); + assertNull(deserialized.getSearchShardTarget()); + assertEquals(querySearchResult.topDocs().maxScore, deserialized.topDocs().maxScore, 0f); + assertEquals(querySearchResult.topDocs().topDocs.totalHits, deserialized.topDocs().topDocs.totalHits); + assertEquals(querySearchResult.from(), deserialized.from()); + assertEquals(querySearchResult.size(), deserialized.size()); + assertEquals(querySearchResult.hasAggs(), deserialized.hasAggs()); + if (deserialized.hasAggs()) { + Aggregations aggs = querySearchResult.consumeAggs().expand(); + Aggregations deserializedAggs = deserialized.consumeAggs().expand(); + assertEquals(aggs.asList(), deserializedAggs.asList()); + } + assertEquals(querySearchResult.terminatedEarly(), deserialized.terminatedEarly()); + assertEquals(querySearchResult.getTookTimeNanos(), deserialized.getTookTimeNanos()); + if (i == 1) { + assertNull(deserialized.getTookTimeNanos()); + } } - assertEquals(querySearchResult.terminatedEarly(), deserialized.terminatedEarly()); - assertEquals(querySearchResult.getTookTimeNanos(), deserialized.getTookTimeNanos()); } public void testNullResponse() throws Exception { From 1d47f38e5d7d485a21f2877cf80ec080bf0237ee Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 31 Oct 2023 11:51:20 -0700 Subject: [PATCH 5/6] Addressed dblock's comment Signed-off-by: Peter Alfonsi --- .../search/query/QuerySearchResultTests.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java b/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java index 4c7770c563764..1b8fc9d7dbc5c 100644 --- a/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java +++ b/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java @@ -56,6 +56,8 @@ import org.opensearch.search.suggest.SuggestTests; import org.opensearch.test.OpenSearchTestCase; +import java.util.HashMap; + import static java.util.Collections.emptyList; public class QuerySearchResultTests extends OpenSearchTestCase { @@ -104,10 +106,13 @@ private static QuerySearchResult createTestInstance() throws Exception { } public void testSerialization() throws Exception { - for (int i = 0; i < 2; i++) { + HashMap expectedValues = new HashMap<>(); // map contains whether to set took time, and if so, to what value + expectedValues.put(false, null); + expectedValues.put(true, 1000L); + for (Boolean doSetTookTime : expectedValues.keySet()) { QuerySearchResult querySearchResult = createTestInstance(); - if (i == 0) { - querySearchResult.setTookTimeNanos(1000L); // test both an initialized and an uninitialized null value + if (doSetTookTime) { + querySearchResult.setTookTimeNanos(expectedValues.get(doSetTookTime)); } QuerySearchResult deserialized = copyWriteable(querySearchResult, namedWriteableRegistry, QuerySearchResult::new); assertEquals(querySearchResult.getContextId().getId(), deserialized.getContextId().getId()); @@ -124,9 +129,7 @@ public void testSerialization() throws Exception { } assertEquals(querySearchResult.terminatedEarly(), deserialized.terminatedEarly()); assertEquals(querySearchResult.getTookTimeNanos(), deserialized.getTookTimeNanos()); - if (i == 1) { - assertNull(deserialized.getTookTimeNanos()); - } + assertEquals(expectedValues.get(doSetTookTime), querySearchResult.getTookTimeNanos()); } } From 81dc9ef9bd38ed120dc372a69326abeace83598c Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Tue, 9 Jan 2024 10:08:40 -0800 Subject: [PATCH 6/6] Addressed Sorabh's comments Signed-off-by: Peter Alfonsi --- .../search/query/QuerySearchResult.java | 13 +++++++------ .../search/query/QuerySearchResultTests.java | 19 +++++++++---------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java b/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java index 2ad601f3bf4a6..6466ee518960a 100644 --- a/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java +++ b/server/src/main/java/org/opensearch/search/query/QuerySearchResult.java @@ -35,6 +35,7 @@ import org.apache.lucene.search.FieldDoc; import org.apache.lucene.search.TotalHits; import org.opensearch.Version; +import org.opensearch.common.SetOnce; import org.opensearch.common.io.stream.DelayableWriteable; import org.opensearch.common.lucene.search.TopDocsAndMaxScore; import org.opensearch.core.common.io.stream.StreamInput; @@ -88,7 +89,7 @@ public final class QuerySearchResult extends SearchPhaseResult { private int nodeQueueSize = -1; private final boolean isNull; - private Long tookTimeNanos = null; + private SetOnce tookTimeNanos = new SetOnce<>(); public QuerySearchResult() { this(false); @@ -367,9 +368,9 @@ public void readFromWithId(ShardSearchContextId id, StreamInput in) throws IOExc setShardSearchRequest(in.readOptionalWriteable(ShardSearchRequest::new)); setRescoreDocIds(new RescoreDocIds(in)); if (in.getVersion().onOrAfter(Version.V_3_0_0)) { - tookTimeNanos = in.readOptionalLong(); + tookTimeNanos = new SetOnce<>(in.readOptionalLong()); } else { - tookTimeNanos = null; + tookTimeNanos = new SetOnce<>(); } } @@ -414,7 +415,7 @@ public void writeToNoId(StreamOutput out) throws IOException { out.writeOptionalWriteable(getShardSearchRequest()); getRescoreDocIds().writeTo(out); if (out.getVersion().onOrAfter(Version.V_3_0_0)) { - out.writeOptionalLong(tookTimeNanos); + out.writeOptionalLong(tookTimeNanos.get()); } } @@ -427,10 +428,10 @@ public float getMaxScore() { } public Long getTookTimeNanos() { - return tookTimeNanos; + return tookTimeNanos.get(); } void setTookTimeNanos(long tookTime) { - tookTimeNanos = tookTime; + tookTimeNanos.set(tookTime); } } diff --git a/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java b/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java index 1b8fc9d7dbc5c..6af1f0b0313d1 100644 --- a/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java +++ b/server/src/test/java/org/opensearch/search/query/QuerySearchResultTests.java @@ -39,6 +39,7 @@ import org.opensearch.action.OriginalIndices; import org.opensearch.action.OriginalIndicesTests; import org.opensearch.action.search.SearchRequest; +import org.opensearch.common.Randomness; import org.opensearch.common.UUIDs; import org.opensearch.common.lucene.search.TopDocsAndMaxScore; import org.opensearch.common.settings.Settings; @@ -57,6 +58,7 @@ import org.opensearch.test.OpenSearchTestCase; import java.util.HashMap; +import java.util.Random; import static java.util.Collections.emptyList; @@ -69,7 +71,7 @@ public QuerySearchResultTests() { this.namedWriteableRegistry = new NamedWriteableRegistry(searchModule.getNamedWriteables()); } - private static QuerySearchResult createTestInstance() throws Exception { + private static QuerySearchResult createTestInstance(boolean setTookTime, Random rand) throws Exception { ShardId shardId = new ShardId("index", "uuid", randomInt()); SearchRequest searchRequest = new SearchRequest().allowPartialSearchResults(randomBoolean()); ShardSearchRequest shardSearchRequest = new ShardSearchRequest( @@ -102,18 +104,16 @@ private static QuerySearchResult createTestInstance() throws Exception { result.aggregations(InternalAggregationsTests.createTestInstance()); } assertNull(result.getTookTimeNanos()); + if (setTookTime) { + result.setTookTimeNanos(rand.nextLong()); + } return result; } public void testSerialization() throws Exception { - HashMap expectedValues = new HashMap<>(); // map contains whether to set took time, and if so, to what value - expectedValues.put(false, null); - expectedValues.put(true, 1000L); - for (Boolean doSetTookTime : expectedValues.keySet()) { - QuerySearchResult querySearchResult = createTestInstance(); - if (doSetTookTime) { - querySearchResult.setTookTimeNanos(expectedValues.get(doSetTookTime)); - } + Random rand = Randomness.get(); + for (Boolean doSetTookTime : new boolean[]{false, true}) { + QuerySearchResult querySearchResult = createTestInstance(doSetTookTime, rand); QuerySearchResult deserialized = copyWriteable(querySearchResult, namedWriteableRegistry, QuerySearchResult::new); assertEquals(querySearchResult.getContextId().getId(), deserialized.getContextId().getId()); assertNull(deserialized.getSearchShardTarget()); @@ -129,7 +129,6 @@ public void testSerialization() throws Exception { } assertEquals(querySearchResult.terminatedEarly(), deserialized.terminatedEarly()); assertEquals(querySearchResult.getTookTimeNanos(), deserialized.getTookTimeNanos()); - assertEquals(expectedValues.get(doSetTookTime), querySearchResult.getTookTimeNanos()); } }