Properly encapsulate SearchRequestOperationsListener related APIs as …

…package protected (internal) (opensearch-project#11315) Signed-off-by: Andriy Redko <[email protected]>
deshsidd · Dec 11, 2023 · fa3e704 · fa3e704
1 parent 24decdc
commit fa3e704
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 29 deletions.
diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java b/server/src/main/java/org/opensearch/action/search/SearchRequestOperationsListener.java
@@ -20,35 +20,35 @@
  * @opensearch.internal
  */
 @InternalApi
-interface SearchRequestOperationsListener {
+abstract class SearchRequestOperationsListener {
 
-    void onPhaseStart(SearchPhaseContext context);
+    abstract void onPhaseStart(SearchPhaseContext context);
 
-    void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext);
+    abstract void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext);
 
-    void onPhaseFailure(SearchPhaseContext context);
+    abstract void onPhaseFailure(SearchPhaseContext context);
 
-    default void onRequestStart(SearchRequestContext searchRequestContext) {}
+    void onRequestStart(SearchRequestContext searchRequestContext) {}
 
-    default void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {}
+    void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {}
 
     /**
      * Holder of Composite Listeners
      *
      * @opensearch.internal
      */
 
-    final class CompositeListener implements SearchRequestOperationsListener {
+    static final class CompositeListener extends SearchRequestOperationsListener {
         private final List<SearchRequestOperationsListener> listeners;
         private final Logger logger;
 
-        public CompositeListener(List<SearchRequestOperationsListener> listeners, Logger logger) {
+        CompositeListener(List<SearchRequestOperationsListener> listeners, Logger logger) {
             this.listeners = listeners;
             this.logger = logger;
         }
 
         @Override
-        public void onPhaseStart(SearchPhaseContext context) {
+        void onPhaseStart(SearchPhaseContext context) {
             for (SearchRequestOperationsListener listener : listeners) {
                 try {
                     listener.onPhaseStart(context);
@@ -59,7 +59,7 @@ public void onPhaseStart(SearchPhaseContext context) {
         }
 
         @Override
-        public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
+        void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
             for (SearchRequestOperationsListener listener : listeners) {
                 try {
                     listener.onPhaseEnd(context, searchRequestContext);
@@ -70,7 +70,7 @@ public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRe
         }
 
         @Override
-        public void onPhaseFailure(SearchPhaseContext context) {
+        void onPhaseFailure(SearchPhaseContext context) {
             for (SearchRequestOperationsListener listener : listeners) {
                 try {
                     listener.onPhaseFailure(context);
@@ -81,7 +81,7 @@ public void onPhaseFailure(SearchPhaseContext context) {
         }
 
         @Override
-        public void onRequestStart(SearchRequestContext searchRequestContext) {
+        void onRequestStart(SearchRequestContext searchRequestContext) {
             for (SearchRequestOperationsListener listener : listeners) {
                 try {
                     listener.onRequestStart(searchRequestContext);

diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java b/server/src/main/java/org/opensearch/action/search/SearchRequestSlowLog.java
@@ -57,7 +57,7 @@
  *
  * @opensearch.internal
  */
-public final class SearchRequestSlowLog implements SearchRequestOperationsListener {
+public final class SearchRequestSlowLog extends SearchRequestOperationsListener {
     private static final Charset UTF_8 = StandardCharsets.UTF_8;
 
     private long warnThreshold;
@@ -134,19 +134,19 @@ public SearchRequestSlowLog(ClusterService clusterService) {
     }
 
     @Override
-    public void onPhaseStart(SearchPhaseContext context) {}
+    void onPhaseStart(SearchPhaseContext context) {}
 
     @Override
-    public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {}
+    void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {}
 
     @Override
-    public void onPhaseFailure(SearchPhaseContext context) {}
+    void onPhaseFailure(SearchPhaseContext context) {}
 
     @Override
-    public void onRequestStart(SearchRequestContext searchRequestContext) {}
+    void onRequestStart(SearchRequestContext searchRequestContext) {}
 
     @Override
-    public void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
+    void onRequestEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
         long tookInNanos = System.nanoTime() - searchRequestContext.getAbsoluteStartNanos();
 
         if (warnThreshold >= 0 && tookInNanos > warnThreshold && level.isLevelEnabledFor(SlowLogLevel.WARN)) {

diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java b/server/src/main/java/org/opensearch/action/search/SearchRequestStats.java
@@ -23,7 +23,7 @@
  * @opensearch.api
  */
 @PublicApi(since = "2.11.0")
-public final class SearchRequestStats implements SearchRequestOperationsListener {
+public final class SearchRequestStats extends SearchRequestOperationsListener {
     Map<SearchPhaseName, StatsHolder> phaseStatsMap = new EnumMap<>(SearchPhaseName.class);
 
     @Inject
@@ -46,20 +46,20 @@ public long getPhaseMetric(SearchPhaseName searchPhaseName) {
     }
 
     @Override
-    public void onPhaseStart(SearchPhaseContext context) {
+    void onPhaseStart(SearchPhaseContext context) {
         phaseStatsMap.get(context.getCurrentPhase().getSearchPhaseName()).current.inc();
     }
 
     @Override
-    public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
+    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()));
     }
 
     @Override
-    public void onPhaseFailure(SearchPhaseContext context) {
+    void onPhaseFailure(SearchPhaseContext context) {
         phaseStatsMap.get(context.getCurrentPhase().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
@@ -300,7 +300,7 @@ private Map<String, Float> resolveIndexBoosts(SearchRequest searchRequest, Clust
      *
      * @opensearch.internal
      */
-    static final class SearchTimeProvider implements SearchRequestOperationsListener {
+    static final class SearchTimeProvider extends SearchRequestOperationsListener {
 
         private final long absoluteStartMillis;
         private final long relativeStartNanos;
@@ -352,18 +352,18 @@ SearchResponse.PhaseTook getPhaseTook() {
         Map<SearchPhaseName, Long> phaseStatsMap = new EnumMap<>(SearchPhaseName.class);
 
         @Override
-        public void onPhaseStart(SearchPhaseContext context) {}
+        void onPhaseStart(SearchPhaseContext context) {}
 
         @Override
-        public void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
+        void onPhaseEnd(SearchPhaseContext context, SearchRequestContext searchRequestContext) {
             phaseStatsMap.put(
                 context.getCurrentPhase().getSearchPhaseName(),
                 TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - context.getCurrentPhase().getStartTimeInNanos())
             );
         }
 
         @Override
-        public void onPhaseFailure(SearchPhaseContext context) {}
+        void onPhaseFailure(SearchPhaseContext context) {}
 
         public Long getPhaseTookTime(SearchPhaseName searchPhaseName) {
             return phaseStatsMap.get(searchPhaseName);

diff --git a/...er/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerSupport.java b/...er/src/test/java/org/opensearch/action/search/SearchRequestOperationsListenerSupport.java
@@ -0,0 +1,22 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.action.search;
+
+/**
+ * Helper interface to access package protected {@link SearchRequestOperationsListener} from test cases.
+ */
+public interface SearchRequestOperationsListenerSupport {
+    default void onPhaseStart(SearchRequestOperationsListener listener, SearchPhaseContext context) {
+        listener.onPhaseStart(context);
+    }
+
+    default void onPhaseEnd(SearchRequestOperationsListener listener, SearchPhaseContext context) {
+        listener.onPhaseEnd(context, new SearchRequestContext());
+    }
+}
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
@@ -35,6 +35,7 @@
 import org.opensearch.action.search.SearchPhase;
 import org.opensearch.action.search.SearchPhaseContext;
 import org.opensearch.action.search.SearchPhaseName;
+import org.opensearch.action.search.SearchRequestOperationsListenerSupport;
 import org.opensearch.action.search.SearchRequestStats;
 import org.opensearch.index.search.stats.SearchStats.Stats;
 import org.opensearch.test.OpenSearchTestCase;
@@ -47,7 +48,7 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-public class SearchStatsTests extends OpenSearchTestCase {
+public class SearchStatsTests extends OpenSearchTestCase implements SearchRequestOperationsListenerSupport {
 
     // https://github.com/elastic/elasticsearch/issues/7644
     public void testShardLevelSearchGroupStats() throws Exception {
@@ -84,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, null /* not needed */);
+                onPhaseStart(testRequestStats, ctx);
+                onPhaseEnd(testRequestStats, ctx);
             }
         }
         searchStats1.setSearchRequestStats(testRequestStats);