From 3572783d62a778b7c7578a6d98fb5cee2bf11b4e Mon Sep 17 00:00:00 2001 From: David Zane Date: Tue, 26 Sep 2023 12:26:22 -0700 Subject: [PATCH] 1st review fixes Signed-off-by: David Zane --- .../action/search/MultiSearchRequest.java | 5 + .../action/search/SearchRequest.java | 12 +- .../action/search/SearchResponse.java | 25 ++- .../search/SearchScrollAsyncAction.java | 2 +- .../action/search/TransportSearchAction.java | 26 ++- .../search/CreatePitControllerTests.java | 2 +- .../action/search/MockSearchPhaseContext.java | 2 +- .../search/MultiSearchResponseTests.java | 4 +- .../action/search/SearchAsyncActionTests.java | 2 +- .../action/search/SearchResponseTests.java | 33 ++-- .../search/SearchTimeProviderTests.java | 187 ++++-------------- .../search/TransportSearchActionTests.java | 2 +- .../search/GenericSearchExtBuilderTests.java | 2 +- 13 files changed, 106 insertions(+), 198 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/MultiSearchRequest.java b/server/src/main/java/org/opensearch/action/search/MultiSearchRequest.java index da8f8f144eaf2..b39f4fbea6464 100644 --- a/server/src/main/java/org/opensearch/action/search/MultiSearchRequest.java +++ b/server/src/main/java/org/opensearch/action/search/MultiSearchRequest.java @@ -277,6 +277,8 @@ public static void readMultiLineFormat( } else if ("cancel_after_time_interval".equals(entry.getKey()) || "cancelAfterTimeInterval".equals(entry.getKey())) { searchRequest.setCancelAfterTimeInterval(nodeTimeValue(value, null)); + } else if ("phase_took".equals(entry.getKey())) { + searchRequest.setPhaseTookQueryParamEnabled(SearchRequest.parseParamValue(value)); } else { throw new IllegalArgumentException("key [" + entry.getKey() + "] is not supported in the metadata section"); } @@ -374,6 +376,9 @@ public static void writeSearchRequestParams(SearchRequest request, XContentBuild if (request.getCancelAfterTimeInterval() != null) { xContentBuilder.field("cancel_after_time_interval", request.getCancelAfterTimeInterval().getStringRep()); } + if (request.isPhaseTookQueryParamEnabled() != null) { + xContentBuilder.field("phase_took", request.isPhaseTookQueryParamEnabled()); + } xContentBuilder.endObject(); } diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequest.java b/server/src/main/java/org/opensearch/action/search/SearchRequest.java index c9d5819e586e7..6106b5a5cbaae 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequest.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequest.java @@ -626,6 +626,16 @@ enum ParamValue { UNSET } + public static Boolean parseParamValue(Object str) { + if ("TRUE".equals(str)) { + return true; + } else if ("FALSE".equals(str)) { + return false; + } else { + return null; + } + } + /** * Returns value of user-provided phase_took query parameter for this search request. * Defaults to false. @@ -643,7 +653,7 @@ public ParamValue isPhaseTookQueryParamEnabled() { /** * Sets value of phase_took query param if provided by user. Defaults to null. */ - public void setPhaseTookQueryParamEnabled(boolean phaseTookQueryParamEnabled) { + public void setPhaseTookQueryParamEnabled(Boolean phaseTookQueryParamEnabled) { this.phaseTookQueryParamEnabled = phaseTookQueryParamEnabled; } diff --git a/server/src/main/java/org/opensearch/action/search/SearchResponse.java b/server/src/main/java/org/opensearch/action/search/SearchResponse.java index de14db5cc187a..3f51e1e9a3cbe 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchResponse.java +++ b/server/src/main/java/org/opensearch/action/search/SearchResponse.java @@ -118,7 +118,7 @@ public SearchResponse(StreamInput in) throws IOException { if (in.getVersion().onOrAfter(Version.V_3_0_0)) { phaseTook = new PhaseTook(in); } else { - phaseTook = PhaseTook.NULL; + phaseTook = PhaseTook.EMPTY; } skippedShards = in.readVInt(); pointInTimeId = in.readOptionalString(); @@ -141,7 +141,7 @@ public SearchResponse( successfulShards, skippedShards, tookInMillis, - SearchResponse.PhaseTook.NULL, + SearchResponse.PhaseTook.EMPTY, shardFailures, clusters, null @@ -326,7 +326,7 @@ public XContentBuilder innerToXContent(XContentBuilder builder, Params params) t builder.field(POINT_IN_TIME_ID.getPreferredName(), pointInTimeId); } builder.field(TOOK.getPreferredName(), tookInMillis); - if (phaseTook.equals(PhaseTook.NULL) == false) { + if (phaseTook.equals(PhaseTook.EMPTY) == false) { phaseTook.toXContent(builder, params); } builder.field(TIMED_OUT.getPreferredName(), isTimedOut()); @@ -368,7 +368,7 @@ public static SearchResponse innerFromXContent(XContentParser parser) throws IOE Boolean terminatedEarly = null; int numReducePhases = 1; long tookInMillis = -1; - PhaseTook phaseTook = PhaseTook.NULL; + PhaseTook phaseTook = PhaseTook.EMPTY; int successfulShards = -1; int totalShards = -1; int skippedShards = 0; // 0 for BWC @@ -441,7 +441,7 @@ public static SearchResponse innerFromXContent(XContentParser parser) throws IOE currentFieldName = parser.currentName(); } else if (token.isValue()) { try { - Enum.valueOf(SearchPhaseName.class, currentFieldName); + SearchPhaseName.valueOf(currentFieldName.toUpperCase()); phaseTookMap.put(currentFieldName, parser.longValue()); } catch (final IllegalArgumentException ex) { parser.skipChildren(); @@ -665,17 +665,18 @@ public String toString() { * @opensearch.internal */ public static class PhaseTook implements ToXContentFragment, Writeable { - public static final PhaseTook NULL = new PhaseTook(); + public static final PhaseTook EMPTY = new PhaseTook(); static final ParseField PHASE_TOOK = new ParseField("phase_took"); private final Map phaseStatsMap; + // Private constructor for empty object private PhaseTook() { - Map nullPhaseTookMap = new HashMap<>(); + Map defaultPhaseTookMap = new HashMap<>(); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - nullPhaseTookMap.put(searchPhaseName.getName(), (long) -1); + defaultPhaseTookMap.put(searchPhaseName.getName(), (long) -1); } - this.phaseStatsMap = nullPhaseTookMap; + this.phaseStatsMap = defaultPhaseTookMap; } public PhaseTook(Map phaseStatsMap) { @@ -688,9 +689,7 @@ private PhaseTook(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - out.writeLong(phaseStatsMap.get(searchPhaseName.getName())); - } + out.writeMap(phaseStatsMap, StreamOutput::writeString, StreamOutput::writeLong); } @Override @@ -747,7 +746,7 @@ static SearchResponse empty(Supplier tookInMillisSupplier, Clusters cluste 0, 0, tookInMillisSupplier.get(), - PhaseTook.NULL, + PhaseTook.EMPTY, ShardSearchFailure.EMPTY_ARRAY, clusters, null diff --git a/server/src/main/java/org/opensearch/action/search/SearchScrollAsyncAction.java b/server/src/main/java/org/opensearch/action/search/SearchScrollAsyncAction.java index 3059b6dbb055c..f65e3c31bf7f8 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchScrollAsyncAction.java +++ b/server/src/main/java/org/opensearch/action/search/SearchScrollAsyncAction.java @@ -299,7 +299,7 @@ protected final void sendResponse( successfulOps.get(), 0, buildTookInMillis(), - SearchResponse.PhaseTook.NULL, + SearchResponse.PhaseTook.EMPTY, buildShardFailures(), SearchResponse.Clusters.EMPTY, null diff --git a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java index b476a342e9232..01941abebf8bb 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -322,7 +322,7 @@ SearchResponse.PhaseTook getPhaseTook() { } return new SearchResponse.PhaseTook(phaseTookMap); } else { - return SearchResponse.PhaseTook.NULL; + return SearchResponse.PhaseTook.EMPTY; } } @@ -341,6 +341,10 @@ public void onPhaseEnd(SearchPhaseContext context) { @Override public void onPhaseFailure(SearchPhaseContext context) {} + + public long getPhaseTookTime(SearchPhaseName searchPhaseName) { + return phaseStatsMap.get(searchPhaseName); + } } @Override @@ -463,15 +467,8 @@ private void executeRequest( relativeStartNanos, System::nanoTime ); - // 2nd executeRequest(), called by 1st executeRequest() - final List searchListenersList = createSearchListenerList(); - if (originalSearchRequest.isPhaseTookQueryParamEnabled() == SearchRequest.ParamValue.TRUE - || (originalSearchRequest.isPhaseTookQueryParamEnabled() == SearchRequest.ParamValue.UNSET - && clusterService.getClusterSettings().get(TransportSearchAction.SEARCH_PHASE_TOOK_ENABLED))) { - timeProvider.setPhaseTookEnabled(true); - searchListenersList.add(timeProvider); - } + final List searchListenersList = createSearchListenerList(originalSearchRequest, timeProvider); final SearchRequestOperationsListener searchRequestOperationsListener; if (!CollectionUtils.isEmpty(searchListenersList)) { @@ -1201,11 +1198,20 @@ AbstractSearchAsyncAction asyncSearchAction( ); } - private List createSearchListenerList() { + private List createSearchListenerList(SearchRequest searchRequest, SearchTimeProvider timeProvider) { final List searchListenersList = new ArrayList<>(); + if (isRequestStatsEnabled) { searchListenersList.add(searchRequestStats); } + + if (searchRequest.isPhaseTookQueryParamEnabled() == SearchRequest.ParamValue.TRUE + || (searchRequest.isPhaseTookQueryParamEnabled() == SearchRequest.ParamValue.UNSET + && clusterService.getClusterSettings().get(TransportSearchAction.SEARCH_PHASE_TOOK_ENABLED))) { + timeProvider.setPhaseTookEnabled(true); + searchListenersList.add(timeProvider); + } + return searchListenersList; } diff --git a/server/src/test/java/org/opensearch/action/search/CreatePitControllerTests.java b/server/src/test/java/org/opensearch/action/search/CreatePitControllerTests.java index 12a3ffcf84ab7..30b71a9504a28 100644 --- a/server/src/test/java/org/opensearch/action/search/CreatePitControllerTests.java +++ b/server/src/test/java/org/opensearch/action/search/CreatePitControllerTests.java @@ -133,7 +133,7 @@ public void setupData() { 3, 0, 100, - SearchResponse.PhaseTook.NULL, + SearchResponse.PhaseTook.EMPTY, ShardSearchFailure.EMPTY_ARRAY, SearchResponse.Clusters.EMPTY, pitId diff --git a/server/src/test/java/org/opensearch/action/search/MockSearchPhaseContext.java b/server/src/test/java/org/opensearch/action/search/MockSearchPhaseContext.java index 9c514323368a8..7fa914a37ce95 100644 --- a/server/src/test/java/org/opensearch/action/search/MockSearchPhaseContext.java +++ b/server/src/test/java/org/opensearch/action/search/MockSearchPhaseContext.java @@ -118,7 +118,7 @@ public void sendSearchResponse(InternalSearchResponse internalSearchResponse, At numSuccess.get(), 0, 0, - SearchResponse.PhaseTook.NULL, + SearchResponse.PhaseTook.EMPTY, failures.toArray(ShardSearchFailure.EMPTY_ARRAY), SearchResponse.Clusters.EMPTY, searchContextId diff --git a/server/src/test/java/org/opensearch/action/search/MultiSearchResponseTests.java b/server/src/test/java/org/opensearch/action/search/MultiSearchResponseTests.java index 325d316d43628..a9d89d66dee90 100644 --- a/server/src/test/java/org/opensearch/action/search/MultiSearchResponseTests.java +++ b/server/src/test/java/org/opensearch/action/search/MultiSearchResponseTests.java @@ -68,7 +68,7 @@ protected MultiSearchResponse createTestInstance() { successfulShards, skippedShards, tookInMillis, - SearchResponse.PhaseTook.NULL, + SearchResponse.PhaseTook.EMPTY, ShardSearchFailure.EMPTY_ARRAY, clusters, null @@ -97,7 +97,7 @@ private static MultiSearchResponse createTestInstanceWithFailures() { successfulShards, skippedShards, tookInMillis, - SearchResponse.PhaseTook.NULL, + SearchResponse.PhaseTook.EMPTY, ShardSearchFailure.EMPTY_ARRAY, clusters, null diff --git a/server/src/test/java/org/opensearch/action/search/SearchAsyncActionTests.java b/server/src/test/java/org/opensearch/action/search/SearchAsyncActionTests.java index d530c6b4e0c3f..dd0fc79cede97 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchAsyncActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchAsyncActionTests.java @@ -713,7 +713,7 @@ public static class TestSearchResponse extends SearchResponse { final Set queried = new HashSet<>(); TestSearchResponse() { - super(InternalSearchResponse.empty(), null, 0, 0, 0, 0L, PhaseTook.NULL, ShardSearchFailure.EMPTY_ARRAY, Clusters.EMPTY, null); + super(InternalSearchResponse.empty(), null, 0, 0, 0, 0L, PhaseTook.EMPTY, ShardSearchFailure.EMPTY_ARRAY, Clusters.EMPTY, null); } } diff --git a/server/src/test/java/org/opensearch/action/search/SearchResponseTests.java b/server/src/test/java/org/opensearch/action/search/SearchResponseTests.java index b4d07cacc44bf..be99c33cff286 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchResponseTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchResponseTests.java @@ -74,7 +74,9 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.UUID; import static java.util.Collections.singletonMap; @@ -152,13 +154,11 @@ public SearchResponse createTestItem( Boolean terminatedEarly = randomBoolean() ? null : randomBoolean(); int numReducePhases = randomIntBetween(1, 10); long tookInMillis = randomNonNegativeLong(); - SearchResponse.PhaseTook phaseTook = new SearchResponse.PhaseTook( - randomNonNegativeLong(), - randomNonNegativeLong(), - randomNonNegativeLong(), - randomNonNegativeLong(), - randomNonNegativeLong() - ); + Map phaseTookMap = new HashMap<>(); + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + phaseTookMap.put(searchPhaseName.getName(), randomNonNegativeLong()); + } + SearchResponse.PhaseTook phaseTook = new SearchResponse.PhaseTook(phaseTookMap); int totalShards = randomIntBetween(1, Integer.MAX_VALUE); int successfulShards = randomIntBetween(0, totalShards); int skippedShards = randomIntBetween(0, totalShards); @@ -328,7 +328,7 @@ public void testToXContent() { 0, 0, 0, - SearchResponse.PhaseTook.NULL, + SearchResponse.PhaseTook.EMPTY, ShardSearchFailure.EMPTY_ARRAY, SearchResponse.Clusters.EMPTY, null @@ -362,6 +362,14 @@ public void testToXContent() { assertEquals(1, searchExtBuilders.size()); } { + Map phaseTookMap = new HashMap<>(); + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + phaseTookMap.put(searchPhaseName.getName(), 0L); + } + phaseTookMap.put(SearchPhaseName.QUERY.getName(), 50L); + phaseTookMap.put(SearchPhaseName.FETCH.getName(), 25L); + phaseTookMap.put(SearchPhaseName.EXPAND.getName(), 30L); + SearchResponse.PhaseTook phaseTook = new SearchResponse.PhaseTook(phaseTookMap); SearchResponse response = new SearchResponse( new InternalSearchResponse( new SearchHits(hits, new TotalHits(100, TotalHits.Relation.EQUAL_TO), 1.5f), @@ -377,7 +385,7 @@ public void testToXContent() { 0, 0, 0, - new SearchResponse.PhaseTook(0, 0, 50, 25, 0), + phaseTook, ShardSearchFailure.EMPTY_ARRAY, new SearchResponse.Clusters(5, 3, 2), null @@ -388,11 +396,12 @@ public void testToXContent() { expectedString.append("\"took\":0,"); expectedString.append("\"phase_took\":"); { - expectedString.append("{\"dfs_prequery\":0,"); - expectedString.append("\"can_match\":0,"); + expectedString.append("{\"dfs_pre_query\":0,"); expectedString.append("\"query\":50,"); expectedString.append("\"fetch\":25,"); - expectedString.append("\"expand_search\":0},"); + expectedString.append("\"dfs_query\":0,"); + expectedString.append("\"expand\":30,"); + expectedString.append("\"can_match\":0},"); } expectedString.append("\"timed_out\":false,"); expectedString.append("\"_shards\":"); diff --git a/server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java b/server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java index aa5c228a67596..80961d5225e73 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java @@ -10,166 +10,45 @@ import org.opensearch.test.OpenSearchTestCase; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.Phaser; +import java.util.concurrent.TimeUnit; -public class SearchTimeProviderTests extends OpenSearchTestCase { - public void testSearchRequestPhaseFailure() { - TransportSearchAction.SearchTimeProvider testRequestStats = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); - SearchPhaseContext ctx = new MockSearchPhaseContext(1); - - testRequestStats.onDFSPreQueryPhaseStart(ctx); - testRequestStats.onDFSPreQueryPhaseFailure(ctx); - assertEquals(0, testRequestStats.getDFSPreQueryTotal()); - - testRequestStats.onCanMatchPhaseStart(ctx); - testRequestStats.onCanMatchPhaseFailure(ctx); - assertEquals(0, testRequestStats.getCanMatchTotal()); - - testRequestStats.onQueryPhaseStart(ctx); - testRequestStats.onQueryPhaseFailure(ctx); - assertEquals(0, testRequestStats.getQueryTotal()); - - testRequestStats.onFetchPhaseStart(ctx); - testRequestStats.onFetchPhaseFailure(ctx); - assertEquals(0, testRequestStats.getFetchTotal()); - - testRequestStats.onExpandSearchPhaseStart(ctx); - testRequestStats.onExpandSearchPhaseFailure(ctx); - assertEquals(0, testRequestStats.getExpandSearchTotal()); - } - - public void testSearchTimeProvider() { - TransportSearchAction.SearchTimeProvider testRequestStats = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); - - SearchPhaseContext ctx = new MockSearchPhaseContext(1); - long tookTime = randomIntBetween(1, 10); - - testRequestStats.onDFSPreQueryPhaseStart(ctx); - testRequestStats.onDFSPreQueryPhaseEnd(ctx, tookTime); - assertEquals(tookTime, testRequestStats.getDFSPreQueryTotal()); - - testRequestStats.onCanMatchPhaseStart(ctx); - testRequestStats.onCanMatchPhaseEnd(ctx, tookTime); - assertEquals(tookTime, testRequestStats.getCanMatchTotal()); - - testRequestStats.onQueryPhaseStart(ctx); - testRequestStats.onQueryPhaseEnd(ctx, tookTime); - assertEquals(tookTime, testRequestStats.getQueryTotal()); +import static org.hamcrest.Matchers.greaterThanOrEqualTo; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; - testRequestStats.onFetchPhaseStart(ctx); - testRequestStats.onFetchPhaseEnd(ctx, tookTime); - testRequestStats.onFetchPhaseEnd(ctx, 10); - assertEquals(tookTime + 10, testRequestStats.getFetchTotal()); - - testRequestStats.onExpandSearchPhaseStart(ctx); - testRequestStats.onExpandSearchPhaseEnd(ctx, tookTime); - testRequestStats.onExpandSearchPhaseEnd(ctx, tookTime); - assertEquals(2 * tookTime, testRequestStats.getExpandSearchTotal()); - } - - public void testSearchTimeProviderOnDFSPreQueryEndConcurrently() throws InterruptedException { - TransportSearchAction.SearchTimeProvider testRequestStats = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); - SearchPhaseContext ctx = new MockSearchPhaseContext(1); - int numTasks = randomIntBetween(5, 50); - long tookTime = randomIntBetween(1, 10); - Thread[] threads = new Thread[numTasks]; - Phaser phaser = new Phaser(numTasks + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks); - for (int i = 0; i < numTasks; i++) { - threads[i] = new Thread(() -> { - phaser.arriveAndAwaitAdvance(); - testRequestStats.onDFSPreQueryPhaseEnd(ctx, tookTime); - countDownLatch.countDown(); - }); - threads[i].start(); - } - phaser.arriveAndAwaitAdvance(); - countDownLatch.await(); - assertEquals(tookTime * numTasks, testRequestStats.getDFSPreQueryTotal()); - } - - public void testSearchTimeProviderOnCanMatchQueryEndConcurrently() throws InterruptedException { - TransportSearchAction.SearchTimeProvider testRequestStats = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); - SearchPhaseContext ctx = new MockSearchPhaseContext(1); - int numTasks = randomIntBetween(5, 50); - long tookTime = randomIntBetween(1, 10); - Thread[] threads = new Thread[numTasks]; - Phaser phaser = new Phaser(numTasks + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks); - for (int i = 0; i < numTasks; i++) { - threads[i] = new Thread(() -> { - phaser.arriveAndAwaitAdvance(); - testRequestStats.onCanMatchPhaseEnd(ctx, tookTime); - countDownLatch.countDown(); - }); - threads[i].start(); - } - phaser.arriveAndAwaitAdvance(); - countDownLatch.await(); - assertEquals(tookTime * numTasks, testRequestStats.getCanMatchTotal()); - } - - public void testSearchTimeProviderOnQueryEndConcurrently() throws InterruptedException { - TransportSearchAction.SearchTimeProvider testRequestStats = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); - SearchPhaseContext ctx = new MockSearchPhaseContext(1); - int numTasks = randomIntBetween(5, 50); - long tookTime = randomIntBetween(1, 10); - Thread[] threads = new Thread[numTasks]; - Phaser phaser = new Phaser(numTasks + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks); - for (int i = 0; i < numTasks; i++) { - threads[i] = new Thread(() -> { - phaser.arriveAndAwaitAdvance(); - testRequestStats.onQueryPhaseEnd(ctx, tookTime); - countDownLatch.countDown(); - }); - threads[i].start(); - } - phaser.arriveAndAwaitAdvance(); - countDownLatch.await(); - assertEquals(tookTime * numTasks, testRequestStats.getQueryTotal()); - } +public class SearchTimeProviderTests extends OpenSearchTestCase { - public void testSearchTimeProviderOnFetchEndConcurrently() throws InterruptedException { - TransportSearchAction.SearchTimeProvider testRequestStats = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); - SearchPhaseContext ctx = new MockSearchPhaseContext(1); - int numTasks = randomIntBetween(5, 50); - long tookTime = randomIntBetween(1, 10); - Thread[] threads = new Thread[numTasks]; - Phaser phaser = new Phaser(numTasks + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks); - for (int i = 0; i < numTasks; i++) { - threads[i] = new Thread(() -> { - phaser.arriveAndAwaitAdvance(); - testRequestStats.onFetchPhaseEnd(ctx, tookTime); - countDownLatch.countDown(); - }); - threads[i].start(); + public void testSearchTimeProviderPhaseFailure() { + TransportSearchAction.SearchTimeProvider testTimeProvider = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); + SearchPhaseContext ctx = mock(SearchPhaseContext.class); + SearchPhase mockSearchPhase = mock(SearchPhase.class); + when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); + + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); + testTimeProvider.onPhaseStart(ctx); + assertEquals(0, testTimeProvider.getPhaseTookTime(searchPhaseName)); + testTimeProvider.onPhaseFailure(ctx); + assertEquals(0, testTimeProvider.getPhaseTookTime(searchPhaseName)); } - phaser.arriveAndAwaitAdvance(); - countDownLatch.await(); - assertEquals(tookTime * numTasks, testRequestStats.getFetchTotal()); } - public void testSearchTimeProviderOnExpandSearchEndConcurrently() throws InterruptedException { - TransportSearchAction.SearchTimeProvider testRequestStats = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); - SearchPhaseContext ctx = new MockSearchPhaseContext(1); - int numTasks = randomIntBetween(5, 50); - long tookTime = randomIntBetween(1, 10); - Thread[] threads = new Thread[numTasks]; - Phaser phaser = new Phaser(numTasks + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks); - for (int i = 0; i < numTasks; i++) { - threads[i] = new Thread(() -> { - phaser.arriveAndAwaitAdvance(); - testRequestStats.onExpandSearchPhaseEnd(ctx, tookTime); - countDownLatch.countDown(); - }); - threads[i].start(); + public void testSearchTimeProviderPhaseEnd() { + TransportSearchAction.SearchTimeProvider testTimeProvider = new TransportSearchAction.SearchTimeProvider(0, 0, () -> 0); + + SearchPhaseContext ctx = mock(SearchPhaseContext.class); + SearchPhase mockSearchPhase = mock(SearchPhase.class); + when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); + + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); + long tookTimeInMillis = randomIntBetween(1, 100); + testTimeProvider.onPhaseStart(ctx); + long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(tookTimeInMillis); + when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime); + assertEquals(0, testTimeProvider.getPhaseTookTime(searchPhaseName)); + testTimeProvider.onPhaseEnd(ctx); + assertThat(testTimeProvider.getPhaseTookTime(searchPhaseName), greaterThanOrEqualTo(tookTimeInMillis)); } - phaser.arriveAndAwaitAdvance(); - countDownLatch.await(); - assertEquals(tookTime * numTasks, testRequestStats.getExpandSearchTotal()); } } diff --git a/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java b/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java index 91b1f5a63b033..4230cff255608 100644 --- a/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java @@ -452,7 +452,7 @@ private static SearchResponse emptySearchResponse() { 1, 0, 100, - SearchResponse.PhaseTook.NULL, + SearchResponse.PhaseTook.EMPTY, ShardSearchFailure.EMPTY_ARRAY, SearchResponse.Clusters.EMPTY, null diff --git a/server/src/test/java/org/opensearch/search/GenericSearchExtBuilderTests.java b/server/src/test/java/org/opensearch/search/GenericSearchExtBuilderTests.java index 2c39c5f2177d9..a87b117f70eab 100644 --- a/server/src/test/java/org/opensearch/search/GenericSearchExtBuilderTests.java +++ b/server/src/test/java/org/opensearch/search/GenericSearchExtBuilderTests.java @@ -264,7 +264,7 @@ public SearchResponse createTestItem( successfulShards, skippedShards, tookInMillis, - SearchResponse.PhaseTook.NULL, + SearchResponse.PhaseTook.EMPTY, shardSearchFailures, randomBoolean() ? randomClusters() : SearchResponse.Clusters.EMPTY, null