From 07623b3c921977eaa80af3d16e7faf50598f9092 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Thu, 16 Nov 2023 17:11:48 -0500 Subject: [PATCH] [BWC and API enforcement] Reduce the visibility of some existing APIs Signed-off-by: Andriy Redko --- .../action/search/SearchPhaseContext.java | 6 ++--- .../action/search/SearchRequestContext.java | 8 +++--- .../SearchRequestOperationsListener.java | 26 +++++++++---------- .../action/search/SearchRequestSlowLog.java | 6 ++--- .../action/search/SearchRequestStats.java | 14 +++++----- .../action/search/TransportSearchAction.java | 11 +++----- .../SearchRequestOperationsListenerTests.java | 16 ++++++------ .../search/SearchRequestStatsTests.java | 16 ++++++------ .../search/SearchTimeProviderTests.java | 8 +++--- .../index/search/stats/SearchStatsTests.java | 4 +-- 10 files changed, 57 insertions(+), 58 deletions(-) diff --git a/server/src/main/java/org/opensearch/action/search/SearchPhaseContext.java b/server/src/main/java/org/opensearch/action/search/SearchPhaseContext.java index 0fa8569413eaf..df451e0745e3c 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchPhaseContext.java +++ b/server/src/main/java/org/opensearch/action/search/SearchPhaseContext.java @@ -34,7 +34,7 @@ import org.apache.logging.log4j.Logger; import org.opensearch.action.OriginalIndices; import org.opensearch.common.Nullable; -import org.opensearch.common.annotation.PublicApi; +import org.opensearch.common.annotation.InternalApi; import org.opensearch.common.lease.Releasable; import org.opensearch.common.util.concurrent.AtomicArray; import org.opensearch.search.SearchPhaseResult; @@ -49,9 +49,9 @@ /** * This class provide contextual state and access to resources across multiple search phases. * - * @opensearch.api + * @opensearch.internal */ -@PublicApi(since = "1.0.0") +@InternalApi public interface SearchPhaseContext extends Executor { // TODO maybe we can make this concrete later - for now we just implement this in the base class for all initial phases diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java b/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java index 96e9d9efa5079..674363600b251 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestContext.java @@ -10,6 +10,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.lucene.search.TotalHits; +import org.opensearch.common.annotation.InternalApi; import java.util.EnumMap; import java.util.HashMap; @@ -22,7 +23,8 @@ * * @opensearch.internal */ -public class SearchRequestContext { +@InternalApi +class SearchRequestContext { private final SearchRequestOperationsListener searchRequestOperationsListener; private long absoluteStartNanos; private final Map phaseTookMap; @@ -32,11 +34,11 @@ public class SearchRequestContext { /** * This constructor is for testing only */ - public SearchRequestContext() { + SearchRequestContext() { this(new SearchRequestOperationsListener.CompositeListener(List.of(), LogManager.getLogger())); } - public SearchRequestContext(SearchRequestOperationsListener searchRequestOperationsListener) { + SearchRequestContext(SearchRequestOperationsListener searchRequestOperationsListener) { this.searchRequestOperationsListener = searchRequestOperationsListener; this.absoluteStartNanos = System.nanoTime(); this.phaseTookMap = new HashMap<>(); diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java index 91e3eecbf8d13..a635d3dea66c9 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java @@ -10,23 +10,23 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; -import org.opensearch.common.annotation.PublicApi; +import org.opensearch.common.annotation.InternalApi; import java.util.List; /** * A listener for search, fetch and context events at the coordinator node level * - * @opensearch.api + * @opensearch.internal */ -@PublicApi(since = "1.0.0") -public interface SearchRequestOperationsListener { +@InternalApi +interface SearchRequestOperationsListener { - void onPhaseStart(SearchPhaseContext context); + void onPhaseStart(SearchPhase phase); - void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext); + void onPhaseEnd(SearchPhase context, SearchRequestContext searchRequestContext); - void onPhaseFailure(SearchPhaseContext context); + void onPhaseFailure(SearchPhase phase); default void onRequestStart(SearchRequestContext searchRequestContext) {} @@ -48,10 +48,10 @@ public CompositeListener(List listeners, Logger } @Override - public void onPhaseStart(SearchPhaseContext context) { + public void onPhaseStart(SearchPhase phase) { for (SearchRequestOperationsListener listener : listeners) { try { - listener.onPhaseStart(context); + listener.onPhaseStart(phase); } catch (Exception e) { logger.warn(() -> new ParameterizedMessage("onPhaseStart listener [{}] failed", listener), e); } @@ -59,10 +59,10 @@ public void onPhaseStart(SearchPhaseContext context) { } @Override - public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) { + public void onPhaseEnd(SearchPhase phase, SearchRequestContext searchRequestContext) { for (SearchRequestOperationsListener listener : listeners) { try { - listener.onPhaseEnd(context, searchRequestContext); + listener.onPhaseEnd(phase, searchRequestContext); } catch (Exception e) { logger.warn(() -> new ParameterizedMessage("onPhaseEnd listener [{}] failed", listener), e); } @@ -70,10 +70,10 @@ public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRe } @Override - public void onPhaseFailure(SearchPhaseContext context) { + public void onPhaseFailure(SearchPhase phase) { for (SearchRequestOperationsListener listener : listeners) { try { - listener.onPhaseFailure(context); + listener.onPhaseFailure(phase); } catch (Exception e) { logger.warn(() -> new ParameterizedMessage("onPhaseFailure listener [{}] failed", listener), e); } diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java index 6a0d60ffd3984..96b621ac52035 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java @@ -134,13 +134,13 @@ public SearchRequestSlowLog(ClusterService clusterService) { } @Override - public void onPhaseStart(SearchPhaseContext context) {} + public void onPhaseStart(SearchPhase phase) {} @Override - public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {} + public void onPhaseEnd(SearchPhase phase, SearchRequestContext searchRequestContext) {} @Override - public void onPhaseFailure(SearchPhaseContext context) {} + public void onPhaseFailure(SearchPhase phase) {} @Override public void onRequestStart(SearchRequestContext searchRequestContext) {} 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 2813c41e043ee..b55a4aeb26632 100644 --- a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java +++ b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java @@ -46,21 +46,21 @@ public long getPhaseMetric(SearchPhaseName searchPhaseName) { } @Override - public void onPhaseStart(SearchPhaseContext context) { - phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.inc(); + public void onPhaseStart(SearchPhase phase) { + phaseStatsMap.get(phase.getSearchPhaseName()).current.inc(); } @Override - public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) { - StatsHolder phaseStats = phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()); + public void onPhaseEnd(SearchPhase phase, SearchRequestContext searchRequestContext) { + StatsHolder phaseStats = phaseStatsMap.get(phase.getSearchPhaseName()); phaseStats.current.dec(); phaseStats.total.inc(); - phaseStats.timing.inc(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos())); + phaseStats.timing.inc(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - phase.getStartTimeInNanos())); } @Override - public void onPhaseFailure(SearchPhaseContext context) { - phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec(); + public void onPhaseFailure(SearchPhase phase) { + phaseStatsMap.get(phase.getSearchPhaseName()).current.dec(); } /** 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 62886f7e9d981..0d58031052035 100644 --- a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java +++ b/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java @@ -352,18 +352,15 @@ SearchResponse.PhaseTook getPhaseTook() { Map phaseStatsMap = new EnumMap<>(SearchPhaseName.class); @Override - public void onPhaseStart(SearchPhaseContext context) {} + public void onPhaseStart(SearchPhase phase) {} @Override - public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) { - phaseStatsMap.put( - context.getCurrentPhase().getSearchPhaseName(), - TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos()) - ); + public void onPhaseEnd(SearchPhase phase, SearchRequestContext searchRequestContext) { + phaseStatsMap.put(phase.getSearchPhaseName(), TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - phase.getStartTimeInNanos())); } @Override - public void onPhaseFailure(SearchPhaseContext context) {} + public void onPhaseFailure(SearchPhase phase) {} public Long getPhaseTookTime(SearchPhaseName searchPhaseName) { return phaseStatsMap.get(searchPhaseName); 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 a53c35a8401b3..80cfdcd09384c 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerTests.java @@ -29,19 +29,19 @@ public void testListenersAreExecuted() { SearchRequestOperationsListener testListener = new SearchRequestOperationsListener() { @Override - public void onPhaseStart(SearchPhaseContext context) { - searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).current.inc(); + public void onPhaseStart(SearchPhase phase) { + searchPhaseMap.get(phase.getSearchPhaseName()).current.inc(); } @Override - public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) { - searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec(); - searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).total.inc(); + public void onPhaseEnd(SearchPhase phase, SearchRequestContext searchRequestContext) { + searchPhaseMap.get(phase.getSearchPhaseName()).current.dec(); + searchPhaseMap.get(phase.getSearchPhaseName()).total.inc(); } @Override - public void onPhaseFailure(SearchPhaseContext context) { - searchPhaseMap.get(context.getCurrentPhase().getSearchPhaseName()).current.dec(); + public void onPhaseFailure(SearchPhase phase) { + searchPhaseMap.get(phase.getSearchPhaseName()).current.dec(); } }; @@ -62,7 +62,7 @@ public void onPhaseFailure(SearchPhaseContext context) { for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { when(ctx.getCurrentPhase()).thenReturn(searchPhase); when(searchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); - compositeListener.onPhaseStart(ctx); + compositeListener.onPhaseStart(ctx.getCurrentPhase()); 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 93cf77933fdd5..5562d1620ed12 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchRequestStatsTests.java @@ -29,9 +29,9 @@ public void testSearchRequestPhaseFailure() { for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); - testRequestStats.onPhaseStart(ctx); + testRequestStats.onPhaseStart(ctx.getCurrentPhase()); assertEquals(1, testRequestStats.getPhaseCurrent(searchPhaseName)); - testRequestStats.onPhaseFailure(ctx); + testRequestStats.onPhaseFailure(ctx.getCurrentPhase()); assertEquals(0, testRequestStats.getPhaseCurrent(searchPhaseName)); } } @@ -46,11 +46,11 @@ public void testSearchRequestStats() { for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); long tookTimeInMillis = randomIntBetween(1, 10); - testRequestStats.onPhaseStart(ctx); + testRequestStats.onPhaseStart(ctx.getCurrentPhase()); long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(tookTimeInMillis); when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime); assertEquals(1, testRequestStats.getPhaseCurrent(searchPhaseName)); - testRequestStats.onPhaseEnd(ctx, new SearchRequestContext()); + testRequestStats.onPhaseEnd(ctx.getCurrentPhase(), new SearchRequestContext()); assertEquals(0, testRequestStats.getPhaseCurrent(searchPhaseName)); assertEquals(1, testRequestStats.getPhaseTotal(searchPhaseName)); assertThat(testRequestStats.getPhaseMetric(searchPhaseName), greaterThanOrEqualTo(tookTimeInMillis)); @@ -71,7 +71,7 @@ public void testSearchRequestStatsOnPhaseStartConcurrently() throws InterruptedE for (int i = 0; i < numTasks; i++) { threads[i] = new Thread(() -> { phaser.arriveAndAwaitAdvance(); - testRequestStats.onPhaseStart(ctx); + testRequestStats.onPhaseStart(ctx.getCurrentPhase()); countDownLatch.countDown(); }); threads[i].start(); @@ -102,7 +102,7 @@ public void testSearchRequestStatsOnPhaseEndConcurrently() throws InterruptedExc for (int i = 0; i < numTasks; i++) { threads[i] = new Thread(() -> { phaser.arriveAndAwaitAdvance(); - testRequestStats.onPhaseEnd(ctx, new SearchRequestContext()); + testRequestStats.onPhaseEnd(ctx.getCurrentPhase(), new SearchRequestContext()); countDownLatch.countDown(); }); threads[i].start(); @@ -134,8 +134,8 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte for (int i = 0; i < numTasks; i++) { threads[i] = new Thread(() -> { phaser.arriveAndAwaitAdvance(); - testRequestStats.onPhaseStart(ctx); - testRequestStats.onPhaseFailure(ctx); + testRequestStats.onPhaseStart(ctx.getCurrentPhase()); + testRequestStats.onPhaseFailure(ctx.getCurrentPhase()); countDownLatch.countDown(); }); threads[i].start(); 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 4d8a44417a3ee..395253d035b82 100644 --- a/server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java +++ b/server/src/test/java/org/opensearch/action/search/SearchTimeProviderTests.java @@ -26,9 +26,9 @@ public void testSearchTimeProviderPhaseFailure() { for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); - testTimeProvider.onPhaseStart(ctx); + testTimeProvider.onPhaseStart(ctx.getCurrentPhase()); assertNull(testTimeProvider.getPhaseTookTime(searchPhaseName)); - testTimeProvider.onPhaseFailure(ctx); + testTimeProvider.onPhaseFailure(ctx.getCurrentPhase()); assertNull(testTimeProvider.getPhaseTookTime(searchPhaseName)); } } @@ -43,11 +43,11 @@ public void testSearchTimeProviderPhaseEnd() { for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); long tookTimeInMillis = randomIntBetween(1, 100); - testTimeProvider.onPhaseStart(ctx); + testTimeProvider.onPhaseStart(ctx.getCurrentPhase()); long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(tookTimeInMillis); when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime); assertNull(testTimeProvider.getPhaseTookTime(searchPhaseName)); - testTimeProvider.onPhaseEnd(ctx, new SearchRequestContext()); + testTimeProvider.onPhaseEnd(ctx.getCurrentPhase(), new SearchRequestContext()); assertThat(testTimeProvider.getPhaseTookTime(searchPhaseName), greaterThanOrEqualTo(tookTimeInMillis)); } } 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 2ebb033899698..f96a591322c78 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 @@ -85,8 +85,8 @@ public void testShardLevelSearchGroupStats() throws Exception { when(mockSearchPhase.getStartTimeInNanos()).thenReturn(System.nanoTime() - TimeUnit.SECONDS.toNanos(paramValue)); when(mockSearchPhase.getSearchPhaseName()).thenReturn(searchPhaseName); for (int iterator = 0; iterator < paramValue; iterator++) { - testRequestStats.onPhaseStart(ctx); - testRequestStats.onPhaseEnd(ctx, new SearchRequestContext()); + testRequestStats.onPhaseStart(ctx.getCurrentPhase()); + testRequestStats.onPhaseEnd(ctx.getCurrentPhase(), new SearchRequestContext()); } } searchStats1.setSearchRequestStats(testRequestStats);