From 052793d4a7344f8b57978c200e6658ad20e3e75c Mon Sep 17 00:00:00 2001 From: David Zane Date: Wed, 27 Sep 2023 15:36:33 -0700 Subject: [PATCH] 2nd review Signed-off-by: David Zane --- .../action/search/MultiSearchRequest.java | 6 ++-- .../action/search/SearchRequest.java | 26 +++++++-------- .../action/search/TransportSearchAction.java | 32 ++++++++++++------- .../rest/action/search/RestSearchAction.java | 4 +-- .../action/search/SearchRequestTests.java | 6 ++-- .../search/RandomSearchRequestGenerator.java | 2 +- 6 files changed, 42 insertions(+), 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 index b39f4fbea6464..9f4207f7a0984 100644 --- 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 index 6106b5a5cbaae..c78dfbd7f1ab1 100644 --- 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,7 @@ public SearchRequest(StreamInput in) throws IOException { if (in.getVersion().onOrAfter(Version.V_2_7_0)) { pipeline = in.readOptionalString(); } - phaseTookQueryParamEnabled = in.readOptionalBoolean(); + phaseTook = in.readOptionalBoolean(); } @Override @@ -288,7 +288,7 @@ public void writeTo(StreamOutput out) throws IOException { if (out.getVersion().onOrAfter(Version.V_2_7_0)) { out.writeOptionalString(pipeline); } - out.writeOptionalBoolean(phaseTookQueryParamEnabled); + out.writeOptionalBoolean(phaseTook); } @Override @@ -640,10 +640,10 @@ public static Boolean parseParamValue(Object str) { * Returns value of user-provided phase_took query parameter for this search request. * Defaults to false. */ - 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 +653,8 @@ public ParamValue isPhaseTookQueryParamEnabled() { /** * Sets value of phase_took query param if provided by user. Defaults to null. */ - public void setPhaseTookQueryParamEnabled(Boolean phaseTookQueryParamEnabled) { - this.phaseTookQueryParamEnabled = phaseTookQueryParamEnabled; + public void setPhaseTook(Boolean phaseTook) { + this.phaseTook = phaseTook; } /** @@ -762,7 +762,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 +784,7 @@ public int hashCode() { absoluteStartMillis, ccsMinimizeRoundtrips, cancelAfterTimeInterval, - phaseTookQueryParamEnabled + phaseTook ); } @@ -827,8 +827,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 index 01941abebf8bb..b8ad8624f62e8 100644 --- 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 phaseTookMap = new HashMap<>(); // Convert Map to Map for SearchResponse() for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) { @@ -1205,11 +1205,21 @@ private List 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 index cbbac35a1590c..080366e536da1 100644 --- 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 index 6e1d6b0b4157c..45cfe3f169c85 100644 --- 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 index c754a5efc4ca7..74de1e6d96d93 100644 --- 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 ra searchRequest.setCancelAfterTimeInterval(TimeValue.parseTimeValue(randomTimeValue(), null, "cancel_after_time_interval")); } if (randomBoolean()) { - searchRequest.setPhaseTookQueryParamEnabled(randomBoolean()); + searchRequest.setPhaseTook(randomBoolean()); } return searchRequest; }