From 0c9110d5f017b0d04033e6cf5f5715aca19f14ef Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 4 Dec 2024 12:48:59 -0800 Subject: [PATCH 1/6] fix create_pit enum bug Signed-off-by: Peter Alfonsi --- .../opensearch/action/search/SearchPhase.java | 10 ++- .../action/search/SearchPhaseName.java | 23 +++-- .../action/search/SearchRequestStats.java | 21 +++-- .../index/search/stats/SearchStats.java | 2 +- .../search/SearchRequestStatsTests.java | 87 +++++++++++++++---- 5 files changed, 108 insertions(+), 35 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchPhase.java b/server/src/main/java/org/opensearch/action/search/SearchPhase.java index 0890e9f5de8d4..9fe6e5f9342ee 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchPhase.java +++ b/server/src/main/java/org/opensearch/action/search/SearchPhase.java @@ -69,11 +69,13 @@ public String getName() { } /** - * Returns the SearchPhase name as {@link SearchPhaseName}. Exception will come if SearchPhase name is not defined - * in {@link SearchPhaseName} - * @return {@link SearchPhaseName} + * Returns the SearchPhase name as {@link SearchPhaseName}. If unrecognized, returns the catch-all OTHER_PHASE_TYPES. */ public SearchPhaseName getSearchPhaseName() { - return SearchPhaseName.valueOf(name.toUpperCase(Locale.ROOT)); + try { + return SearchPhaseName.valueOf(name.toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + return SearchPhaseName.OTHER_PHASE_TYPES; + } } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java b/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java index 8cf92934c8a52..853e0fab88d9a 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java +++ b/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java @@ -17,20 +17,29 @@ */ @PublicApi(since = "2.9.0") public enum SearchPhaseName { - DFS_PRE_QUERY("dfs_pre_query"), - QUERY("query"), - FETCH("fetch"), - DFS_QUERY("dfs_query"), - EXPAND("expand"), - CAN_MATCH("can_match"); + DFS_PRE_QUERY("dfs_pre_query", true), + QUERY("query", true), + FETCH("fetch", true), + DFS_QUERY("dfs_query", true), + EXPAND("expand", true), + CAN_MATCH("can_match", true), + + // A catch-all for other phase types which shouldn't appear in the search phase stats API. + OTHER_PHASE_TYPES("other_phase_types", false); private final String name; + private final boolean shouldTrack; - SearchPhaseName(final String name) { + SearchPhaseName(final String name, final boolean shouldTrack) { this.name = name; + this.shouldTrack = shouldTrack; } public String getName() { return name; } + + public boolean shouldTrack() { + return shouldTrack; + } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java index 94200d29a4f21..cb0a742c6f775 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java @@ -73,20 +73,29 @@ public long getTookMetric() { @Override protected void onPhaseStart(SearchPhaseContext context) { - phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.inc(); + SearchPhaseName phaseName = context.getCurrentPhase().getSearchPhaseName(); + if (phaseName.shouldTrack()) { + phaseStatsMap.get(phaseName).current.inc(); + } } @Override protected void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) { - StatsHolder phaseStats = phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()); - phaseStats.current.dec(); - phaseStats.total.inc(); - phaseStats.timing.inc(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos())); + SearchPhaseName phaseName = context.getCurrentPhase().getSearchPhaseName(); + if (phaseName.shouldTrack()) { + StatsHolder phaseStats = phaseStatsMap.get(phaseName); + phaseStats.current.dec(); + phaseStats.total.inc(); + phaseStats.timing.inc(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos())); + } } @Override protected void onPhaseFailure(SearchPhaseContext context, Throwable cause) { - phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec(); + SearchPhaseName phaseName = context.getCurrentPhase().getSearchPhaseName(); + if (phaseName.shouldTrack()) { + phaseStatsMap.get(phaseName).current.dec(); + } } @Override diff --git a/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java b/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java index d6ea803c9ee13..f5b4bb38be9b0 100644 --- a/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java +++ b/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java @@ -524,7 +524,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { PhaseStatsLongHolder statsLongHolder = requestStatsLongHolder.requestStatsHolder.get(searchPhaseName.getName()); - if (statsLongHolder == null) { + if (statsLongHolder == null || !searchPhaseName.shouldTrack()) { continue; } builder.startObject(searchPhaseName.getName()); diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java index 3bad3ec3e7d21..5a0d33af51956 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java @@ -13,6 +13,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.test.OpenSearchTestCase; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -25,6 +26,18 @@ import static org.mockito.Mockito.when; public class SearchRequestStatsTests extends OpenSearchTestCase { + + static List trackablePhases; + + static { + trackablePhases = new ArrayList<>(); + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + if (searchPhaseName.shouldTrack()) { + trackablePhases.add(searchPhaseName); + } + } + } + public void testSearchRequestStats_OnRequestFailure() { ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); @@ -67,7 +80,7 @@ public void testSearchRequestPhaseFailure() { SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + for (SearchPhaseName searchPhaseName : trackablePhases) { when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); testRequestStats.onPhaseStart(ctx); assertEquals(1, testRequestStats.getPhaseCurrent(searchPhaseName)); @@ -84,7 +97,7 @@ public void testSearchRequestStats() { SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + for (SearchPhaseName searchPhaseName : trackablePhases) { when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); long tookTimeInMillis = randomIntBetween(1, 10); testRequestStats.onPhaseStart(ctx); @@ -109,10 +122,10 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); int numTasks = randomIntBetween(5, 50); - Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; - Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks * SearchPhaseName.values().length); - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + Thread[] threads = new Thread[numTasks * trackablePhases.size()]; + Phaser phaser = new Phaser(numTasks * trackablePhases.size() + 1); + CountDownLatch countDownLatch = new CountDownLatch(numTasks * trackablePhases.size()); + for (SearchPhaseName searchPhaseName : trackablePhases) { SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -128,7 +141,7 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE } phaser.arriveAndAwaitAdvance(); countDownLatch.await(); - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + for (SearchPhaseName searchPhaseName : trackablePhases) { assertEquals(numTasks, testRequestStats.getPhaseCurrent(searchPhaseName)); } } @@ -137,11 +150,11 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); int numTasks = randomIntBetween(5, 50); - Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; - Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks * SearchPhaseName.values().length); + Thread[] threads = new Thread[numTasks * trackablePhases.size()]; + Phaser phaser = new Phaser(numTasks * trackablePhases.size() + 1); + CountDownLatch countDownLatch = new CountDownLatch(numTasks * trackablePhases.size()); Map searchPhaseNameLongMap = new HashMap<>(); - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + for (SearchPhaseName searchPhaseName : trackablePhases) { SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -168,7 +181,7 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc } phaser.arriveAndAwaitAdvance(); countDownLatch.await(); - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + for (SearchPhaseName searchPhaseName : trackablePhases) { assertEquals(numTasks, testRequestStats.getPhaseTotal(searchPhaseName)); assertThat( testRequestStats.getPhaseMetric(searchPhaseName), @@ -181,10 +194,10 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); int numTasks = randomIntBetween(5, 50); - Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; - Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks * SearchPhaseName.values().length); - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + Thread[] threads = new Thread[numTasks * trackablePhases.size()]; + Phaser phaser = new Phaser(numTasks * trackablePhases.size() + 1); + CountDownLatch countDownLatch = new CountDownLatch(numTasks * trackablePhases.size()); + for (SearchPhaseName searchPhaseName : trackablePhases) { SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -201,8 +214,48 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte } phaser.arriveAndAwaitAdvance(); countDownLatch.await(); - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + for (SearchPhaseName searchPhaseName : trackablePhases) { assertEquals(0, testRequestStats.getPhaseCurrent(searchPhaseName)); } } + + public void testOtherPhaseNamesAreIgnored() { + // Unrecognized phase names shouldn't be tracked, but should not throw any error. + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); + SearchPhaseContext ctx = mock(SearchPhaseContext.class); + SearchPhase mockSearchPhase = mock(SearchPhase.class); + when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); + + when(mockSearchPhase.getSearchPhaseName()).thenReturn(SearchPhaseName.OTHER_PHASE_TYPES); + testRequestStats.onPhaseStart(ctx); + long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(10); + when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime); + // All values should return 0 for untracked phase types + assertEquals(0, testRequestStats.getPhaseCurrent(SearchPhaseName.OTHER_PHASE_TYPES)); + testRequestStats.onPhaseEnd( + ctx, + new SearchRequestContext( + new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger()), + new SearchRequest(), + () -> null + ) + ); + assertEquals(0, testRequestStats.getPhaseCurrent(SearchPhaseName.OTHER_PHASE_TYPES)); + assertEquals(0, testRequestStats.getPhaseTotal(SearchPhaseName.OTHER_PHASE_TYPES)); + assertEquals(0, testRequestStats.getPhaseMetric(SearchPhaseName.OTHER_PHASE_TYPES)); + } + + public void testSearchPhaseCatchAll() { + // Test search phases with unrecognized names return the catch-all OTHER_PHASE_TYPES when getSearchPhaseName() is called. + // These may exist, for example, "create_pit". + String unrecognizedName = "unrecognized_name"; + SearchPhase dummyPhase = new SearchPhase(unrecognizedName) { + @Override + public void run() {} + }; + + assertEquals(unrecognizedName, dummyPhase.getName()); + assertEquals(SearchPhaseName.OTHER_PHASE_TYPES, dummyPhase.getSearchPhaseName()); + } } From 6d35c7e14472449e3a40c0fd998e2f4f31458853 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 4 Dec 2024 13:00:01 -0800 Subject: [PATCH 2/6] changelog Signed-off-by: Peter Alfonsi --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cc1b4a363050..b5ac80b1df861 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - [Tiered Caching] Fix bug in cache stats API ([#16560](https://github.com/opensearch-project/OpenSearch/pull/16560)) - Bound the size of cache in deprecation logger ([16702](https://github.com/opensearch-project/OpenSearch/issues/16702)) - Ensure consistency of system flag on IndexMetadata after diff is applied ([#16644](https://github.com/opensearch-project/OpenSearch/pull/16644)) +- Fix illegal argument exception when creating a PIT ([#16781](https://github.com/opensearch-project/OpenSearch/pull/16781)) ### Security From ea30484370116fbba655c4b2a29e1ebe7b0ea3ef Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Wed, 4 Dec 2024 13:53:22 -0800 Subject: [PATCH 3/6] Fix SearchResponse XContent Signed-off-by: Peter Alfonsi --- .../main/java/org/opensearch/action/search/SearchResponse.java | 1 + .../org/opensearch/index/search/stats/SearchStatsTests.java | 2 ++ 2 files changed, 3 insertions(+) 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 899c71e91e3ab..7131d4611ea79 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchResponse.java +++ b/server/src/main/java/org/opensearch/action/search/SearchResponse.java @@ -705,6 +705,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.startObject(PHASE_TOOK.getPreferredName()); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + if (!searchPhaseName.shouldTrack()) continue; if (phaseTookMap.containsKey(searchPhaseName.getName())) { builder.field(searchPhaseName.getName(), phaseTookMap.get(searchPhaseName.getName())); } else { diff --git a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java index 594700ea60b3e..bb8896023fdc7 100644 --- a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java +++ b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java @@ -83,6 +83,7 @@ public void testShardLevelSearchGroupStats() throws Exception { SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); SearchPhaseContext ctx = mock(SearchPhaseContext.class); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + if (!searchPhaseName.shouldTrack()) continue; SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); when(mockSearchPhase.getStartTimeInNanos()).thenReturn(System.nanoTime() - TimeUnit.SECONDS.toNanos(paramValue)); @@ -94,6 +95,7 @@ public void testShardLevelSearchGroupStats() throws Exception { } searchStats1.setSearchRequestStats(testRequestStats); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { + if (!searchPhaseName.shouldTrack()) continue; assertEquals( 0, searchStats1.getTotal().getRequestStatsLongHolder().getRequestStatsHolder().get(searchPhaseName.getName()).current From a41f9be10026344d956f2b861d2439b9c03f2d5b Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Thu, 5 Dec 2024 16:40:26 -0800 Subject: [PATCH 4/6] Addressed David's comments Signed-off-by: Peter Alfonsi --- .../opensearch/action/search/SearchPhase.java | 10 +-- .../action/search/SearchPhaseName.java | 23 ++---- .../action/search/SearchRequestStats.java | 21 +++--- .../action/search/SearchResponse.java | 1 - .../index/search/stats/SearchStats.java | 2 +- .../search/SearchRequestStatsTests.java | 74 ++++++------------- .../index/search/stats/SearchStatsTests.java | 2 - 7 files changed, 45 insertions(+), 88 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchPhase.java b/server/src/main/java/org/opensearch/action/search/SearchPhase.java index 9fe6e5f9342ee..0890e9f5de8d4 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchPhase.java +++ b/server/src/main/java/org/opensearch/action/search/SearchPhase.java @@ -69,13 +69,11 @@ public String getName() { } /** - * Returns the SearchPhase name as {@link SearchPhaseName}. If unrecognized, returns the catch-all OTHER_PHASE_TYPES. + * Returns the SearchPhase name as {@link SearchPhaseName}. Exception will come if SearchPhase name is not defined + * in {@link SearchPhaseName} + * @return {@link SearchPhaseName} */ public SearchPhaseName getSearchPhaseName() { - try { - return SearchPhaseName.valueOf(name.toUpperCase(Locale.ROOT)); - } catch (IllegalArgumentException e) { - return SearchPhaseName.OTHER_PHASE_TYPES; - } + return SearchPhaseName.valueOf(name.toUpperCase(Locale.ROOT)); } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java b/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java index 853e0fab88d9a..8cf92934c8a52 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java +++ b/server/src/main/java/org/opensearch/action/search/SearchPhaseName.java @@ -17,29 +17,20 @@ */ @PublicApi(since = "2.9.0") public enum SearchPhaseName { - DFS_PRE_QUERY("dfs_pre_query", true), - QUERY("query", true), - FETCH("fetch", true), - DFS_QUERY("dfs_query", true), - EXPAND("expand", true), - CAN_MATCH("can_match", true), - - // A catch-all for other phase types which shouldn't appear in the search phase stats API. - OTHER_PHASE_TYPES("other_phase_types", false); + DFS_PRE_QUERY("dfs_pre_query"), + QUERY("query"), + FETCH("fetch"), + DFS_QUERY("dfs_query"), + EXPAND("expand"), + CAN_MATCH("can_match"); private final String name; - private final boolean shouldTrack; - SearchPhaseName(final String name, final boolean shouldTrack) { + SearchPhaseName(final String name) { this.name = name; - this.shouldTrack = shouldTrack; } public String getName() { return name; } - - public boolean shouldTrack() { - return shouldTrack; - } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java index cb0a742c6f775..a2722318ac599 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java @@ -73,28 +73,31 @@ public long getTookMetric() { @Override protected void onPhaseStart(SearchPhaseContext context) { - SearchPhaseName phaseName = context.getCurrentPhase().getSearchPhaseName(); - if (phaseName.shouldTrack()) { - phaseStatsMap.get(phaseName).current.inc(); + try { + phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.inc(); + } catch (IllegalArgumentException ignored) { + // Do nothing if the phase isn't found in SearchPhaseName. } } @Override protected void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) { - SearchPhaseName phaseName = context.getCurrentPhase().getSearchPhaseName(); - if (phaseName.shouldTrack()) { - StatsHolder phaseStats = phaseStatsMap.get(phaseName); + try { + StatsHolder phaseStats = phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()); phaseStats.current.dec(); phaseStats.total.inc(); phaseStats.timing.inc(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos())); + } catch (IllegalArgumentException ignored) { + // Do nothing if the phase isn't found in SearchPhaseName. } } @Override protected void onPhaseFailure(SearchPhaseContext context, Throwable cause) { - SearchPhaseName phaseName = context.getCurrentPhase().getSearchPhaseName(); - if (phaseName.shouldTrack()) { - phaseStatsMap.get(phaseName).current.dec(); + try { + phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec(); + } catch (IllegalArgumentException ignored) { + // Do nothing if the phase isn't found in SearchPhaseName. } } 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 7131d4611ea79..899c71e91e3ab 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchResponse.java +++ b/server/src/main/java/org/opensearch/action/search/SearchResponse.java @@ -705,7 +705,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.startObject(PHASE_TOOK.getPreferredName()); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - if (!searchPhaseName.shouldTrack()) continue; if (phaseTookMap.containsKey(searchPhaseName.getName())) { builder.field(searchPhaseName.getName(), phaseTookMap.get(searchPhaseName.getName())); } else { diff --git a/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java b/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java index f5b4bb38be9b0..d6ea803c9ee13 100644 --- a/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java +++ b/server/src/main/java/org/opensearch/index/search/stats/SearchStats.java @@ -524,7 +524,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { PhaseStatsLongHolder statsLongHolder = requestStatsLongHolder.requestStatsHolder.get(searchPhaseName.getName()); - if (statsLongHolder == null || !searchPhaseName.shouldTrack()) { + if (statsLongHolder == null) { continue; } builder.startObject(searchPhaseName.getName()); diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java index 5a0d33af51956..2b9a5992c0117 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java @@ -13,7 +13,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.test.OpenSearchTestCase; -import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -26,18 +25,6 @@ import static org.mockito.Mockito.when; public class SearchRequestStatsTests extends OpenSearchTestCase { - - static List trackablePhases; - - static { - trackablePhases = new ArrayList<>(); - for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - if (searchPhaseName.shouldTrack()) { - trackablePhases.add(searchPhaseName); - } - } - } - public void testSearchRequestStats_OnRequestFailure() { ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); @@ -80,7 +67,7 @@ public void testSearchRequestPhaseFailure() { SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); testRequestStats.onPhaseStart(ctx); assertEquals(1, testRequestStats.getPhaseCurrent(searchPhaseName)); @@ -97,7 +84,7 @@ public void testSearchRequestStats() { SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); long tookTimeInMillis = randomIntBetween(1, 10); testRequestStats.onPhaseStart(ctx); @@ -122,10 +109,10 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); int numTasks = randomIntBetween(5, 50); - Thread[] threads = new Thread[numTasks * trackablePhases.size()]; - Phaser phaser = new Phaser(numTasks * trackablePhases.size() + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks * trackablePhases.size()); - for (SearchPhaseName searchPhaseName : trackablePhases) { + Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; + Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); + CountDownLatch countDownLatch = new CountDownLatch(numTasks * SearchPhaseName.values().length); + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -141,7 +128,7 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE } phaser.arriveAndAwaitAdvance(); countDownLatch.await(); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { assertEquals(numTasks, testRequestStats.getPhaseCurrent(searchPhaseName)); } } @@ -150,11 +137,11 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); int numTasks = randomIntBetween(5, 50); - Thread[] threads = new Thread[numTasks * trackablePhases.size()]; - Phaser phaser = new Phaser(numTasks * trackablePhases.size() + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks * trackablePhases.size()); + Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; + Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); + CountDownLatch countDownLatch = new CountDownLatch(numTasks * SearchPhaseName.values().length); Map searchPhaseNameLongMap = new HashMap<>(); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -181,7 +168,7 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc } phaser.arriveAndAwaitAdvance(); countDownLatch.await(); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { assertEquals(numTasks, testRequestStats.getPhaseTotal(searchPhaseName)); assertThat( testRequestStats.getPhaseMetric(searchPhaseName), @@ -194,10 +181,10 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); int numTasks = randomIntBetween(5, 50); - Thread[] threads = new Thread[numTasks * trackablePhases.size()]; - Phaser phaser = new Phaser(numTasks * trackablePhases.size() + 1); - CountDownLatch countDownLatch = new CountDownLatch(numTasks * trackablePhases.size()); - for (SearchPhaseName searchPhaseName : trackablePhases) { + Thread[] threads = new Thread[numTasks * SearchPhaseName.values().length]; + Phaser phaser = new Phaser(numTasks * SearchPhaseName.values().length + 1); + CountDownLatch countDownLatch = new CountDownLatch(numTasks * SearchPhaseName.values().length); + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); @@ -214,7 +201,7 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte } phaser.arriveAndAwaitAdvance(); countDownLatch.await(); - for (SearchPhaseName searchPhaseName : trackablePhases) { + for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { assertEquals(0, testRequestStats.getPhaseCurrent(searchPhaseName)); } } @@ -224,15 +211,12 @@ public void testOtherPhaseNamesAreIgnored() { ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); SearchPhaseContext ctx = mock(SearchPhaseContext.class); - SearchPhase mockSearchPhase = mock(SearchPhase.class); + SearchPhase mockSearchPhase = new SearchPhase("unrecognized_phase") { + @Override + public void run() {} + }; when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - - when(mockSearchPhase.getSearchPhaseName()).thenReturn(SearchPhaseName.OTHER_PHASE_TYPES); testRequestStats.onPhaseStart(ctx); - long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(10); - when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime); - // All values should return 0 for untracked phase types - assertEquals(0, testRequestStats.getPhaseCurrent(SearchPhaseName.OTHER_PHASE_TYPES)); testRequestStats.onPhaseEnd( ctx, new SearchRequestContext( @@ -241,21 +225,5 @@ public void testOtherPhaseNamesAreIgnored() { () -> null ) ); - assertEquals(0, testRequestStats.getPhaseCurrent(SearchPhaseName.OTHER_PHASE_TYPES)); - assertEquals(0, testRequestStats.getPhaseTotal(SearchPhaseName.OTHER_PHASE_TYPES)); - assertEquals(0, testRequestStats.getPhaseMetric(SearchPhaseName.OTHER_PHASE_TYPES)); - } - - public void testSearchPhaseCatchAll() { - // Test search phases with unrecognized names return the catch-all OTHER_PHASE_TYPES when getSearchPhaseName() is called. - // These may exist, for example, "create_pit". - String unrecognizedName = "unrecognized_name"; - SearchPhase dummyPhase = new SearchPhase(unrecognizedName) { - @Override - public void run() {} - }; - - assertEquals(unrecognizedName, dummyPhase.getName()); - assertEquals(SearchPhaseName.OTHER_PHASE_TYPES, dummyPhase.getSearchPhaseName()); } } diff --git a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java index bb8896023fdc7..594700ea60b3e 100644 --- a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java +++ b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java @@ -83,7 +83,6 @@ public void testShardLevelSearchGroupStats() throws Exception { SearchRequestStats testRequestStats = new SearchRequestStats(clusterSettings); SearchPhaseContext ctx = mock(SearchPhaseContext.class); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - if (!searchPhaseName.shouldTrack()) continue; SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); when(mockSearchPhase.getStartTimeInNanos()).thenReturn(System.nanoTime() - TimeUnit.SECONDS.toNanos(paramValue)); @@ -95,7 +94,6 @@ public void testShardLevelSearchGroupStats() throws Exception { } searchStats1.setSearchRequestStats(testRequestStats); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - if (!searchPhaseName.shouldTrack()) continue; assertEquals( 0, searchStats1.getTotal().getRequestStatsLongHolder().getRequestStatsHolder().get(searchPhaseName.getName()).current From e20a4de2ce39efa2ecc1c71f31fd4d6e082c627f Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Fri, 6 Dec 2024 08:51:05 -0800 Subject: [PATCH 5/6] rerun gradle Signed-off-by: Peter Alfonsi From 49fec5734f5f1bb366764b689780c38833b90a84 Mon Sep 17 00:00:00 2001 From: Peter Alfonsi Date: Mon, 9 Dec 2024 12:29:54 -0800 Subject: [PATCH 6/6] Addressed andrross's comment Signed-off-by: Peter Alfonsi --- .../opensearch/action/search/SearchPhase.java | 15 ++++-- .../action/search/SearchRequestStats.java | 20 ++------ .../AbstractSearchAsyncActionTests.java | 50 +++++++++---------- .../SearchRequestOperationsListenerTests.java | 11 ++-- .../search/SearchRequestStatsTests.java | 11 ++-- .../index/search/stats/SearchStatsTests.java | 3 +- 6 files changed, 54 insertions(+), 56 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchPhase.java b/server/src/main/java/org/opensearch/action/search/SearchPhase.java index 0890e9f5de8d4..8eab2ee8dedac 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchPhase.java +++ b/server/src/main/java/org/opensearch/action/search/SearchPhase.java @@ -37,6 +37,7 @@ import java.io.IOException; import java.util.Locale; import java.util.Objects; +import java.util.Optional; /** * Base class for all individual search phases like collecting distributed frequencies, fetching documents, querying shards. @@ -69,11 +70,15 @@ public String getName() { } /** - * Returns the SearchPhase name as {@link SearchPhaseName}. Exception will come if SearchPhase name is not defined - * in {@link SearchPhaseName} - * @return {@link SearchPhaseName} + * Returns an Optional of the SearchPhase name as {@link SearchPhaseName}. If there's not a matching SearchPhaseName, + * returns an empty Optional. + * @return {@link Optional} */ - public SearchPhaseName getSearchPhaseName() { - return SearchPhaseName.valueOf(name.toUpperCase(Locale.ROOT)); + public Optional getSearchPhaseName() { + try { + return Optional.of(SearchPhaseName.valueOf(name.toUpperCase(Locale.ROOT))); + } catch (IllegalArgumentException e) { + return Optional.empty(); + } } } diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java index a2722318ac599..88728436df847 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java @@ -73,32 +73,22 @@ public long getTookMetric() { @Override protected void onPhaseStart(SearchPhaseContext context) { - try { - phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.inc(); - } catch (IllegalArgumentException ignored) { - // Do nothing if the phase isn't found in SearchPhaseName. - } + context.getCurrentPhase().getSearchPhaseName().ifPresent(name -> phaseStatsMap.get(name).current.inc()); } @Override protected void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) { - try { - StatsHolder phaseStats = phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()); + context.getCurrentPhase().getSearchPhaseName().ifPresent(name -> { + StatsHolder phaseStats = phaseStatsMap.get(name); phaseStats.current.dec(); phaseStats.total.inc(); phaseStats.timing.inc(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos())); - } catch (IllegalArgumentException ignored) { - // Do nothing if the phase isn't found in SearchPhaseName. - } + }); } @Override protected void onPhaseFailure(SearchPhaseContext context, Throwable cause) { - try { - phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec(); - } catch (IllegalArgumentException ignored) { - // Do nothing if the phase isn't found in SearchPhaseName. - } + context.getCurrentPhase().getSearchPhaseName().ifPresent(name -> phaseStatsMap.get(name).current.dec()); } @Override diff --git a/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java b/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java index 27336e86e52b0..3bc9282e17fc4 100644 --- a/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java +++ b/server/src/test/java/org/opensearch/action/search/AbstractSearchAsyncActionTests.java @@ -399,29 +399,29 @@ public void testOnPhaseFailureAndVerifyListeners() { final List requestOperationListeners = List.of(testListener, assertingListener); SearchQueryThenFetchAsyncAction action = createSearchQueryThenFetchAsyncAction(requestOperationListeners); action.start(); - assertEquals(1, testListener.getPhaseCurrent(action.getSearchPhaseName())); + assertEquals(1, testListener.getPhaseCurrent(action.getSearchPhaseName().get())); action.onPhaseFailure(new SearchPhase("test") { @Override public void run() { } }, "message", null); - assertEquals(0, testListener.getPhaseCurrent(action.getSearchPhaseName())); - assertEquals(0, testListener.getPhaseTotal(action.getSearchPhaseName())); + assertEquals(0, testListener.getPhaseCurrent(action.getSearchPhaseName().get())); + assertEquals(0, testListener.getPhaseTotal(action.getSearchPhaseName().get())); SearchDfsQueryThenFetchAsyncAction searchDfsQueryThenFetchAsyncAction = createSearchDfsQueryThenFetchAsyncAction( requestOperationListeners ); searchDfsQueryThenFetchAsyncAction.start(); - assertEquals(1, testListener.getPhaseCurrent(searchDfsQueryThenFetchAsyncAction.getSearchPhaseName())); + assertEquals(1, testListener.getPhaseCurrent(searchDfsQueryThenFetchAsyncAction.getSearchPhaseName().get())); searchDfsQueryThenFetchAsyncAction.onPhaseFailure(new SearchPhase("test") { @Override public void run() { } }, "message", null); - assertEquals(0, testListener.getPhaseCurrent(action.getSearchPhaseName())); - assertEquals(0, testListener.getPhaseTotal(action.getSearchPhaseName())); + assertEquals(0, testListener.getPhaseCurrent(action.getSearchPhaseName().get())); + assertEquals(0, testListener.getPhaseTotal(action.getSearchPhaseName().get())); FetchSearchPhase fetchPhase = createFetchSearchPhase(); ShardId shardId = new ShardId(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLength(10), randomInt()); @@ -430,15 +430,15 @@ public void run() { action.skipShard(searchShardIterator); action.start(); action.executeNextPhase(action, fetchPhase); - assertEquals(1, testListener.getPhaseCurrent(fetchPhase.getSearchPhaseName())); + assertEquals(1, testListener.getPhaseCurrent(fetchPhase.getSearchPhaseName().get())); action.onPhaseFailure(new SearchPhase("test") { @Override public void run() { } }, "message", null); - assertEquals(0, testListener.getPhaseCurrent(fetchPhase.getSearchPhaseName())); - assertEquals(0, testListener.getPhaseTotal(fetchPhase.getSearchPhaseName())); + assertEquals(0, testListener.getPhaseCurrent(fetchPhase.getSearchPhaseName().get())); + assertEquals(0, testListener.getPhaseTotal(fetchPhase.getSearchPhaseName().get())); } public void testOnPhaseFailure() { @@ -722,7 +722,7 @@ public void testOnPhaseListenersWithQueryAndThenFetchType() throws InterruptedEx action.start(); // Verify queryPhase current metric - assertEquals(1, testListener.getPhaseCurrent(action.getSearchPhaseName())); + assertEquals(1, testListener.getPhaseCurrent(action.getSearchPhaseName().get())); TimeUnit.MILLISECONDS.sleep(delay); FetchSearchPhase fetchPhase = createFetchSearchPhase(); @@ -733,12 +733,12 @@ public void testOnPhaseListenersWithQueryAndThenFetchType() throws InterruptedEx action.executeNextPhase(action, fetchPhase); // Verify queryPhase total, current and latency metrics - assertEquals(0, testListener.getPhaseCurrent(action.getSearchPhaseName())); - assertThat(testListener.getPhaseMetric(action.getSearchPhaseName()), greaterThanOrEqualTo(delay)); - assertEquals(1, testListener.getPhaseTotal(action.getSearchPhaseName())); + assertEquals(0, testListener.getPhaseCurrent(action.getSearchPhaseName().get())); + assertThat(testListener.getPhaseMetric(action.getSearchPhaseName().get()), greaterThanOrEqualTo(delay)); + assertEquals(1, testListener.getPhaseTotal(action.getSearchPhaseName().get())); // Verify fetchPhase current metric - assertEquals(1, testListener.getPhaseCurrent(fetchPhase.getSearchPhaseName())); + assertEquals(1, testListener.getPhaseCurrent(fetchPhase.getSearchPhaseName().get())); TimeUnit.MILLISECONDS.sleep(delay); ExpandSearchPhase expandPhase = createExpandSearchPhase(); @@ -746,18 +746,18 @@ public void testOnPhaseListenersWithQueryAndThenFetchType() throws InterruptedEx TimeUnit.MILLISECONDS.sleep(delay); // Verify fetchPhase total, current and latency metrics - assertThat(testListener.getPhaseMetric(fetchPhase.getSearchPhaseName()), greaterThanOrEqualTo(delay)); - assertEquals(1, testListener.getPhaseTotal(fetchPhase.getSearchPhaseName())); - assertEquals(0, testListener.getPhaseCurrent(fetchPhase.getSearchPhaseName())); + assertThat(testListener.getPhaseMetric(fetchPhase.getSearchPhaseName().get()), greaterThanOrEqualTo(delay)); + assertEquals(1, testListener.getPhaseTotal(fetchPhase.getSearchPhaseName().get())); + assertEquals(0, testListener.getPhaseCurrent(fetchPhase.getSearchPhaseName().get())); - assertEquals(1, testListener.getPhaseCurrent(expandPhase.getSearchPhaseName())); + assertEquals(1, testListener.getPhaseCurrent(expandPhase.getSearchPhaseName().get())); action.executeNextPhase(expandPhase, fetchPhase); action.onPhaseDone(); /* finish phase since we don't have reponse being sent */ - assertThat(testListener.getPhaseMetric(expandPhase.getSearchPhaseName()), greaterThanOrEqualTo(delay)); - assertEquals(1, testListener.getPhaseTotal(expandPhase.getSearchPhaseName())); - assertEquals(0, testListener.getPhaseCurrent(expandPhase.getSearchPhaseName())); + assertThat(testListener.getPhaseMetric(expandPhase.getSearchPhaseName().get()), greaterThanOrEqualTo(delay)); + assertEquals(1, testListener.getPhaseTotal(expandPhase.getSearchPhaseName().get())); + assertEquals(0, testListener.getPhaseCurrent(expandPhase.getSearchPhaseName().get())); } public void testOnPhaseListenersWithDfsType() throws InterruptedException { @@ -772,7 +772,7 @@ public void testOnPhaseListenersWithDfsType() throws InterruptedException { FetchSearchPhase fetchPhase = createFetchSearchPhase(); searchDfsQueryThenFetchAsyncAction.start(); - assertEquals(1, testListener.getPhaseCurrent(searchDfsQueryThenFetchAsyncAction.getSearchPhaseName())); + assertEquals(1, testListener.getPhaseCurrent(searchDfsQueryThenFetchAsyncAction.getSearchPhaseName().get())); TimeUnit.MILLISECONDS.sleep(delay); ShardId shardId = new ShardId(randomAlphaOfLengthBetween(5, 10), randomAlphaOfLength(10), randomInt()); SearchShardIterator searchShardIterator = new SearchShardIterator(null, shardId, Collections.emptyList(), OriginalIndices.NONE); @@ -786,9 +786,9 @@ public void testOnPhaseListenersWithDfsType() throws InterruptedException { null ); /* finalizing the fetch phase since we do adhoc phase lifecycle calls */ - assertThat(testListener.getPhaseMetric(searchDfsQueryThenFetchAsyncAction.getSearchPhaseName()), greaterThanOrEqualTo(delay)); - assertEquals(1, testListener.getPhaseTotal(searchDfsQueryThenFetchAsyncAction.getSearchPhaseName())); - assertEquals(0, testListener.getPhaseCurrent(searchDfsQueryThenFetchAsyncAction.getSearchPhaseName())); + assertThat(testListener.getPhaseMetric(searchDfsQueryThenFetchAsyncAction.getSearchPhaseName().get()), greaterThanOrEqualTo(delay)); + assertEquals(1, testListener.getPhaseTotal(searchDfsQueryThenFetchAsyncAction.getSearchPhaseName().get())); + assertEquals(0, testListener.getPhaseCurrent(searchDfsQueryThenFetchAsyncAction.getSearchPhaseName().get())); } private SearchDfsQueryThenFetchAsyncAction createSearchDfsQueryThenFetchAsyncAction( diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerTests.java index 990ed95f1aebc..0b62d5a16427a 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerTests.java @@ -14,6 +14,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -30,18 +31,18 @@ public void testListenersAreExecuted() { @Override public void onPhaseStart(SearchPhaseContext context) { - searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).current.inc(); + searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName().get()).current.inc(); } @Override public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) { - searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec(); - searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).total.inc(); + searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName().get()).current.dec(); + searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName().get()).total.inc(); } @Override public void onPhaseFailure(SearchPhaseContext context, Throwable cause) { - searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec(); + searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName().get()).current.dec(); } }; @@ -61,7 +62,7 @@ public void onPhaseFailure(SearchPhaseContext context, Throwable cause) { for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { when(ctx.getCurrentPhase()).thenReturn(searchPhase); - when(searchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); + when(searchPhase.getSearchPhaseName()).thenReturn(Optional.of(searchPhaseName)); compositeListener.onPhaseStart(ctx); assertEquals(totalListeners, searchPhaseMap.get(searchPhaseName).current.count()); } diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java index 2b9a5992c0117..876bc395dcd52 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java @@ -16,6 +16,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Phaser; import java.util.concurrent.TimeUnit; @@ -68,7 +69,7 @@ public void testSearchRequestPhaseFailure() { when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); + when(mockSearchPhase.getSearchPhaseName()).thenReturn(Optional.of(searchPhaseName)); testRequestStats.onPhaseStart(ctx); assertEquals(1, testRequestStats.getPhaseCurrent(searchPhaseName)); testRequestStats.onPhaseFailure(ctx, new Throwable()); @@ -85,7 +86,7 @@ public void testSearchRequestStats() { when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { - when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); + when(mockSearchPhase.getSearchPhaseName()).thenReturn(Optional.of(searchPhaseName)); long tookTimeInMillis = randomIntBetween(1, 10); testRequestStats.onPhaseStart(ctx); long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(tookTimeInMillis); @@ -116,7 +117,7 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); + when(mockSearchPhase.getSearchPhaseName()).thenReturn(Optional.of(searchPhaseName)); for (int i = 0; i < numTasks; i++) { threads[i] = new Thread(() -> { phaser.arriveAndAwaitAdvance(); @@ -145,7 +146,7 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); + when(mockSearchPhase.getSearchPhaseName()).thenReturn(Optional.of(searchPhaseName)); long tookTimeInMillis = randomIntBetween(1, 10); long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(tookTimeInMillis); when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime); @@ -188,7 +189,7 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte SearchPhaseContext ctx = mock(SearchPhaseContext.class); SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); - when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); + when(mockSearchPhase.getSearchPhaseName()).thenReturn(Optional.of(searchPhaseName)); for (int i = 0; i < numTasks; i++) { threads[i] = new Thread(() -> { phaser.arriveAndAwaitAdvance(); diff --git a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java index 594700ea60b3e..519c937e348bc 100644 --- a/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java +++ b/server/src/test/java/org/opensearch/index/search/stats/SearchStatsTests.java @@ -44,6 +44,7 @@ import java.util.HashMap; import java.util.Map; +import java.util.Optional; import java.util.concurrent.TimeUnit; import static org.hamcrest.Matchers.greaterThanOrEqualTo; @@ -86,7 +87,7 @@ public void testShardLevelSearchGroupStats() throws Exception { SearchPhase mockSearchPhase = mock(SearchPhase.class); when(ctx.getCurrentPhase()).thenReturn(mockSearchPhase); when(mockSearchPhase.getStartTimeInNanos()).thenReturn(System.nanoTime() - TimeUnit.SECONDS.toNanos(paramValue)); - when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); + when(mockSearchPhase.getSearchPhaseName()).thenReturn(Optional.of(searchPhaseName)); for (int iterator = 0; iterator < paramValue; iterator++) { onPhaseStart(testRequestStats, ctx); onPhaseEnd(testRequestStats, ctx);