2nd review

Signed-off-by: David Zane <[email protected]>

server/src/main/java/org/opensearch/action/search/MultiSearchRequest.java

@@ -278,7 +278,7 @@ public static void readMultiLineFormat(
                             || "cancelAfterTimeInterval".equals(entry.getKey())) {
                                 searchRequest.setCancelAfterTimeInterval(nodeTimeValue(value, null));
                             } else if ("phase_took".equals(entry.getKey())) {
-                                searchRequest.setPhaseTookQueryParamEnabled(SearchRequest.parseParamValue(value));
+                                searchRequest.setPhaseTook(SearchRequest.parseParamValue(value));
                             } else {
                                 throw new IllegalArgumentException("key [" + entry.getKey() + "] is not supported in the metadata section");
                             }
@@ -376,8 +376,8 @@ 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());
+        if (request.isPhaseTook() != null) {
+            xContentBuilder.field("phase_took", request.isPhaseTook());
         }
         xContentBuilder.endObject();
     }

server/src/main/java/org/opensearch/action/search/SearchRequest.java

@@ -117,7 +117,7 @@ public class SearchRequest extends ActionRequest implements IndicesRequest.Repla
 
     private String pipeline;
 
-    private Boolean phaseTookQueryParamEnabled = null;
+    private Boolean phaseTook = null;
 
     public SearchRequest() {
         this.localClusterAlias = null;
@@ -211,7 +211,7 @@ private SearchRequest(
         this.absoluteStartMillis = absoluteStartMillis;
         this.finalReduce = finalReduce;
         this.cancelAfterTimeInterval = searchRequest.cancelAfterTimeInterval;
-        this.phaseTookQueryParamEnabled = searchRequest.phaseTookQueryParamEnabled;
+        this.phaseTook = searchRequest.phaseTook;
     }
 
     /**
@@ -256,7 +256,9 @@ public SearchRequest(StreamInput in) throws IOException {
         if (in.getVersion().onOrAfter(Version.V_2_7_0)) {
             pipeline = in.readOptionalString();
         }
-        phaseTookQueryParamEnabled = in.readOptionalBoolean();
+        if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
+            phaseTook = in.readOptionalBoolean();
+        }
     }
 
     @Override
@@ -288,7 +290,9 @@ public void writeTo(StreamOutput out) throws IOException {
         if (out.getVersion().onOrAfter(Version.V_2_7_0)) {
             out.writeOptionalString(pipeline);
         }
-        out.writeOptionalBoolean(phaseTookQueryParamEnabled);
+        if (out.getVersion().onOrAfter(Version.V_3_0_0)) {
+            out.writeOptionalBoolean(phaseTook);
+        }
     }
 
     @Override
@@ -640,10 +644,10 @@ public static Boolean parseParamValue(Object str) {
      * Returns value of user-provided phase_took query parameter for this search request.
      * Defaults to <code>false</code>.
      */
-    public ParamValue isPhaseTookQueryParamEnabled() {
-        if (phaseTookQueryParamEnabled == null) {
+    public ParamValue isPhaseTook() {
+        if (phaseTook == null) {
             return ParamValue.UNSET;
-        } else if (phaseTookQueryParamEnabled == true) {
+        } else if (phaseTook == true) {
             return ParamValue.TRUE;
         } else {
             return ParamValue.FALSE;
@@ -653,8 +657,8 @@ public ParamValue isPhaseTookQueryParamEnabled() {
     /**
      * Sets value of phase_took query param if provided by user. Defaults to <code>null</code>.
      */
-    public void setPhaseTookQueryParamEnabled(Boolean phaseTookQueryParamEnabled) {
-        this.phaseTookQueryParamEnabled = phaseTookQueryParamEnabled;
+    public void setPhaseTook(Boolean phaseTook) {
+        this.phaseTook = phaseTook;
     }
 
     /**
@@ -762,7 +766,7 @@ public boolean equals(Object o) {
             && ccsMinimizeRoundtrips == that.ccsMinimizeRoundtrips
             && Objects.equals(cancelAfterTimeInterval, that.cancelAfterTimeInterval)
             && Objects.equals(pipeline, that.pipeline)
-            && Objects.equals(phaseTookQueryParamEnabled, that.phaseTookQueryParamEnabled);
+            && Objects.equals(phaseTook, that.phaseTook);
     }
 
     @Override
@@ -784,7 +788,7 @@ public int hashCode() {
             absoluteStartMillis,
             ccsMinimizeRoundtrips,
             cancelAfterTimeInterval,
-            phaseTookQueryParamEnabled
+            phaseTook
         );
     }
 
@@ -827,8 +831,8 @@ public String toString() {
             + cancelAfterTimeInterval
             + ", pipeline="
             + pipeline
-            + ", phaseTookQueryParamEnabled="
-            + phaseTookQueryParamEnabled
+            + ", phaseTook="
+            + phaseTook
             + "}";
     }
 }

server/src/main/java/org/opensearch/action/search/SearchResponse.java

@@ -116,9 +116,9 @@ public SearchResponse(StreamInput in) throws IOException {
         scrollId = in.readOptionalString();
         tookInMillis = in.readVLong();
         if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
-            phaseTook = new PhaseTook(in);
+            phaseTook = in.readOptionalWriteable(PhaseTook::new);
         } else {
-            phaseTook = PhaseTook.EMPTY;
+            phaseTook = null;
         }
         skippedShards = in.readVInt();
         pointInTimeId = in.readOptionalString();
@@ -133,6 +133,20 @@ public SearchResponse(
         long tookInMillis,
         ShardSearchFailure[] shardFailures,
         Clusters clusters
+    ) {
+        this(internalResponse, scrollId, totalShards, successfulShards, skippedShards, tookInMillis, null, shardFailures, clusters, null);
+    }
+
+    public SearchResponse(
+        SearchResponseSections internalResponse,
+        String scrollId,
+        int totalShards,
+        int successfulShards,
+        int skippedShards,
+        long tookInMillis,
+        ShardSearchFailure[] shardFailures,
+        Clusters clusters,
+        String pointInTimeId
     ) {
         this(
             internalResponse,
@@ -141,10 +155,10 @@ public SearchResponse(
             successfulShards,
             skippedShards,
             tookInMillis,
-            SearchResponse.PhaseTook.EMPTY,
+            null,
             shardFailures,
             clusters,
-            null
+            pointInTimeId
         );
     }
 
@@ -326,7 +340,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.EMPTY) == false) {
+        if (phaseTook != null) {
             phaseTook.toXContent(builder, params);
         }
         builder.field(TIMED_OUT.getPreferredName(), isTimedOut());
@@ -368,7 +382,7 @@ public static SearchResponse innerFromXContent(XContentParser parser) throws IOE
         Boolean terminatedEarly = null;
         int numReducePhases = 1;
         long tookInMillis = -1;
-        PhaseTook phaseTook = PhaseTook.EMPTY;
+        PhaseTook phaseTook = null;
         int successfulShards = -1;
         int totalShards = -1;
         int skippedShards = 0; // 0 for BWC
@@ -543,7 +557,7 @@ public void writeTo(StreamOutput out) throws IOException {
         out.writeOptionalString(scrollId);
         out.writeVLong(tookInMillis);
         if (out.getVersion().onOrAfter(Version.V_3_0_0)) {
-            phaseTook.writeTo(out);
+            out.writeOptionalWriteable(phaseTook);
         }
         out.writeVInt(skippedShards);
         out.writeOptionalString(pointInTimeId);
@@ -665,20 +679,9 @@ public String toString() {
      * @opensearch.internal
      */
     public static class PhaseTook implements ToXContentFragment, Writeable {
-        public static final PhaseTook EMPTY = new PhaseTook();
-
         static final ParseField PHASE_TOOK = new ParseField("phase_took");
         private final Map<String, Long> phaseStatsMap;
 
-        // Private constructor for empty object
-        private PhaseTook() {
-            Map<String, Long> defaultPhaseTookMap = new HashMap<>();
-            for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
-                defaultPhaseTookMap.put(searchPhaseName.getName(), (long) -1);
-            }
-            this.phaseStatsMap = defaultPhaseTookMap;
-        }
-
         public PhaseTook(Map<String, Long> phaseStatsMap) {
             this.phaseStatsMap = phaseStatsMap;
         }
@@ -746,7 +749,6 @@ static SearchResponse empty(Supplier<Long> tookInMillisSupplier, Clusters cluste
             0,
             0,
             tookInMillisSupplier.get(),
-            PhaseTook.EMPTY,
             ShardSearchFailure.EMPTY_ARRAY,
             clusters,
             null

server/src/main/java/org/opensearch/action/search/SearchScrollAsyncAction.java

@@ -299,7 +299,6 @@ protected final void sendResponse(
                     successfulOps.get(),
                     0,
                     buildTookInMillis(),
-                    SearchResponse.PhaseTook.EMPTY,
                     buildShardFailures(),
                     SearchResponse.Clusters.EMPTY,
                     null

server/src/main/java/org/opensearch/action/search/TransportSearchAction.java

@@ -275,7 +275,7 @@ static final class SearchTimeProvider implements SearchRequestOperationsListener
         private final long absoluteStartMillis;
         private final long relativeStartNanos;
         private final LongSupplier relativeCurrentNanosProvider;
-        private boolean phaseTookEnabled = false;
+        private boolean phaseTook = false;
 
         /**
          * Instantiates a new search time provider. The absolute start time is the real clock time
@@ -305,24 +305,24 @@ long buildTookInMillis() {
             return TimeUnit.NANOSECONDS.toMillis(relativeCurrentNanosProvider.getAsLong() - relativeStartNanos);
         }
 
-        public void setPhaseTookEnabled(boolean phaseTookEnabled) {
-            this.phaseTookEnabled = phaseTookEnabled;
+        public void setPhaseTook(boolean phaseTook) {
+            this.phaseTook = phaseTook;
         }
 
-        public boolean isPhaseTookEnabled() {
-            return phaseTookEnabled;
+        public boolean isPhaseTook() {
+            return phaseTook;
         }
 
         SearchResponse.PhaseTook getPhaseTook() {
-            if (phaseTookEnabled) {
+            if (phaseTook) {
                 Map<String, Long> phaseTookMap = new HashMap<>();
                 // Convert Map<SearchPhaseName, Long> to Map<String, Long> for SearchResponse()
                 for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
                     phaseTookMap.put(searchPhaseName.getName(), phaseStatsMap.get(searchPhaseName));
                 }
                 return new SearchResponse.PhaseTook(phaseTookMap);
             } else {
-                return SearchResponse.PhaseTook.EMPTY;
+                return null;
             }
         }
 
@@ -1205,11 +1205,21 @@ private List<SearchRequestOperationsListener> createSearchListenerList(SearchReq
             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);
+        // phase_took is enabled with request param and/or cluster setting
+        // check cluster setting only when request param is unspecified
+        switch (searchRequest.isPhaseTook()) {
+            case TRUE:
+                timeProvider.setPhaseTook(true);
+                searchListenersList.add(timeProvider);
+                break;
+            case FALSE:
+                break;
+            case UNSET:
+                if (clusterService.getClusterSettings().get(TransportSearchAction.SEARCH_PHASE_TOOK_ENABLED)) {
+                    timeProvider.setPhaseTook(true);
+                    searchListenersList.add(timeProvider);
+                }
+                break;
         }
 
         return searchListenersList;

server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java

@@ -182,8 +182,8 @@ public static void parseSearchRequest(
 
         if (request.hasParam("phase_took")) {
             // only set if we have the parameter passed to override the cluster-level default
-            // else phaseTookQueryParamEnabled = null
-            searchRequest.setPhaseTookQueryParamEnabled(request.paramAsBoolean("phase_took", true));
+            // else phaseTook = null
+            searchRequest.setPhaseTook(request.paramAsBoolean("phase_took", true));
         }
 
         // do not allow 'query_and_fetch' or 'dfs_query_and_fetch' search types

server/src/test/java/org/opensearch/action/search/CreatePitControllerTests.java

@@ -133,7 +133,6 @@ public void setupData() {
             3,
             0,
             100,
-            SearchResponse.PhaseTook.EMPTY,
             ShardSearchFailure.EMPTY_ARRAY,
             SearchResponse.Clusters.EMPTY,
             pitId

server/src/test/java/org/opensearch/action/search/MockSearchPhaseContext.java

@@ -118,7 +118,6 @@ public void sendSearchResponse(InternalSearchResponse internalSearchResponse, At
                 numSuccess.get(),
                 0,
                 0,
-                SearchResponse.PhaseTook.EMPTY,
                 failures.toArray(ShardSearchFailure.EMPTY_ARRAY),
                 SearchResponse.Clusters.EMPTY,
                 searchContextId

server/src/test/java/org/opensearch/action/search/MultiSearchResponseTests.java

@@ -68,7 +68,6 @@ protected MultiSearchResponse createTestInstance() {
                 successfulShards,
                 skippedShards,
                 tookInMillis,
-                SearchResponse.PhaseTook.EMPTY,
                 ShardSearchFailure.EMPTY_ARRAY,
                 clusters,
                 null
@@ -97,7 +96,6 @@ private static MultiSearchResponse createTestInstanceWithFailures() {
                     successfulShards,
                     skippedShards,
                     tookInMillis,
-                    SearchResponse.PhaseTook.EMPTY,
                     ShardSearchFailure.EMPTY_ARRAY,
                     clusters,
                     null

server/src/test/java/org/opensearch/action/search/SearchAsyncActionTests.java

@@ -713,7 +713,7 @@ public static class TestSearchResponse extends SearchResponse {
         final Set<ShardId> queried = new HashSet<>();
 
         TestSearchResponse() {
-            super(InternalSearchResponse.empty(), null, 0, 0, 0, 0L, PhaseTook.EMPTY, ShardSearchFailure.EMPTY_ARRAY, Clusters.EMPTY, null);
+            super(InternalSearchResponse.empty(), null, 0, 0, 0, 0L, ShardSearchFailure.EMPTY_ARRAY, Clusters.EMPTY, null);
         }
     }
 

server/src/test/java/org/opensearch/action/search/SearchRequestTests.java

@@ -107,7 +107,6 @@ public void testRandomVersionSerialization() throws IOException {
         Version version = VersionUtils.randomVersion(random());
         SearchRequest deserializedRequest = copyWriteable(searchRequest, namedWriteableRegistry, SearchRequest::new, version);
         assertEquals(searchRequest.isCcsMinimizeRoundtrips(), deserializedRequest.isCcsMinimizeRoundtrips());
-        assertEquals(searchRequest.isPhaseTookQueryParamEnabled(), deserializedRequest.isPhaseTookQueryParamEnabled());
         assertEquals(searchRequest.getLocalClusterAlias(), deserializedRequest.getLocalClusterAlias());
         assertEquals(searchRequest.getAbsoluteStartMillis(), deserializedRequest.getAbsoluteStartMillis());
         assertEquals(searchRequest.isFinalReduce(), deserializedRequest.isFinalReduce());
@@ -245,9 +244,7 @@ private SearchRequest mutate(SearchRequest searchRequest) {
         );
         mutators.add(() -> mutation.source(randomValueOtherThan(searchRequest.source(), this::createSearchSourceBuilder)));
         mutators.add(() -> mutation.setCcsMinimizeRoundtrips(searchRequest.isCcsMinimizeRoundtrips() == false));
-        mutators.add(
-            () -> mutation.setPhaseTookQueryParamEnabled(searchRequest.isPhaseTookQueryParamEnabled() == SearchRequest.ParamValue.FALSE)
-        );
+        mutators.add(() -> mutation.setPhaseTook(searchRequest.isPhaseTook() == SearchRequest.ParamValue.FALSE));
         mutators.add(
             () -> mutation.setCancelAfterTimeInterval(
                 searchRequest.getCancelAfterTimeInterval() != null

server/src/test/java/org/opensearch/action/search/SearchResponseTests.java

@@ -328,7 +328,6 @@ public void testToXContent() {
                 0,
                 0,
                 0,
-                SearchResponse.PhaseTook.EMPTY,
                 ShardSearchFailure.EMPTY_ARRAY,
                 SearchResponse.Clusters.EMPTY,
                 null

server/src/test/java/org/opensearch/action/search/TransportSearchActionTests.java

@@ -445,18 +445,7 @@ private static SearchResponse emptySearchResponse() {
             null,
             1
         );
-        return new SearchResponse(
-            response,
-            null,
-            1,
-            1,
-            0,
-            100,
-            SearchResponse.PhaseTook.EMPTY,
-            ShardSearchFailure.EMPTY_ARRAY,
-            SearchResponse.Clusters.EMPTY,
-            null
-        );
+        return new SearchResponse(response, null, 1, 1, 0, 100, ShardSearchFailure.EMPTY_ARRAY, SearchResponse.Clusters.EMPTY, null);
     }
 
     public void testCCSRemoteReduceMergeFails() throws Exception {

server/src/test/java/org/opensearch/search/GenericSearchExtBuilderTests.java

@@ -264,7 +264,6 @@ public SearchResponse createTestItem(
             successfulShards,
             skippedShards,
             tookInMillis,
-            SearchResponse.PhaseTook.EMPTY,
             shardSearchFailures,
             randomBoolean() ? randomClusters() : SearchResponse.Clusters.EMPTY,
             null