2nd review

Signed-off-by: David Zane <[email protected]>
dzane17 · Sep 29, 2023 · 141f514 · 141f514
1 parent 3572783
commit 141f514
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 34 deletions.
diff --git a/server/src/main/java/org/opensearch/action/search/MultiSearchRequest.java b/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();
     }

diff --git a/server/src/main/java/org/opensearch/action/search/SearchRequest.java b/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
             + "}";
     }
 }
diff --git a/server/src/main/java/org/opensearch/action/search/TransportSearchAction.java b/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,16 +305,16 @@ 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()) {
@@ -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;

diff --git a/server/src/main/java/org/opensearch/rest/action/search/RestSearchAction.java b/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

diff --git a/server/src/test/java/org/opensearch/action/search/SearchRequestTests.java b/server/src/test/java/org/opensearch/action/search/SearchRequestTests.java
@@ -107,7 +107,7 @@ 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.isPhaseTook(), deserializedRequest.isPhaseTook());
         assertEquals(searchRequest.getLocalClusterAlias(), deserializedRequest.getLocalClusterAlias());
         assertEquals(searchRequest.getAbsoluteStartMillis(), deserializedRequest.getAbsoluteStartMillis());
         assertEquals(searchRequest.isFinalReduce(), deserializedRequest.isFinalReduce());
@@ -245,9 +245,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

diff --git a/test/framework/src/main/java/org/opensearch/search/RandomSearchRequestGenerator.java b/test/framework/src/main/java/org/opensearch/search/RandomSearchRequestGenerator.java
@@ -132,7 +132,7 @@ public static SearchRequest randomSearchRequest(Supplier<SearchSourceBuilder> ra
             searchRequest.setCancelAfterTimeInterval(TimeValue.parseTimeValue(randomTimeValue(), null, "cancel_after_time_interval"));
         }
         if (randomBoolean()) {
-            searchRequest.setPhaseTookQueryParamEnabled(randomBoolean());
+            searchRequest.setPhaseTook(randomBoolean());
         }
         return searchRequest;
     }