Fix create_pit bug with catch-all search phase name

Signed-off-by: Peter Alfonsi <[email protected]>
peteralfonsi · Jan 21, 2025 · 0e6fcb9 · 0e6fcb9
1 parent 454616e
commit 0e6fcb9
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 3 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
@@ -74,6 +74,10 @@ public String getName() {
      * @return {@link SearchPhaseName}
      */
     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
@@ -22,7 +22,12 @@ public enum SearchPhaseName {
     FETCH("fetch"),
     DFS_QUERY("dfs_query"),
     EXPAND("expand"),
-    CAN_MATCH("can_match");
+    CAN_MATCH("can_match"),
+
+    /**
+     A catch-all for other phase types which shouldn't appear in the search phase stats API.
+     */
+    OTHER_PHASE_TYPES("other_phase_types");
 
     private final String name;
 

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
@@ -524,7 +524,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
 
                 for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
                     PhaseStatsLongHolder statsLongHolder = requestStatsLongHolder.requestStatsHolder.get(searchPhaseName.getName());
-                    if (statsLongHolder == null) {
+                    // The catch-all OTHER_PHASE_TYPES should not appear in the API response.
+                    if (statsLongHolder == null || searchPhaseName.equals(SearchPhaseName.OTHER_PHASE_TYPES)) {
                         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
@@ -205,4 +205,46 @@ public void testSearchRequestStatsOnPhaseFailureConcurrently() throws Interrupte
             assertEquals(0, testRequestStats.getPhaseCurrent(searchPhaseName));
         }
     }
+
+    public void testOtherPhaseNamesAreIgnored() {
+        // Unrecognized phase names 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);
+        int minTimeNanos = 10;
+        long startTime = System.nanoTime() - TimeUnit.MILLISECONDS.toNanos(minTimeNanos);
+        when(mockSearchPhase.getStartTimeInNanos()).thenReturn(startTime);
+
+        assertEquals(1, 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(1, testRequestStats.getPhaseTotal(SearchPhaseName.OTHER_PHASE_TYPES));
+        assertTrue(minTimeNanos <= 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());
+    }
 }