Skip to content

Commit

Permalink
1st review fixes
Browse files Browse the repository at this point in the history 
Signed-off-by: David Zane <[email protected]>
  • Loading branch information
@dzane17
dzane17 committed Sep 26, 2023
1 parent 331ae88 commit 3572783
Show file tree
Hide file tree
Showing 13 changed files with 106 additions and 198 deletions.
5 changes: 5 additions & 0 deletions server/src/main/java/org/opensearch/action/search/MultiSearchRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,8 @@ public static void readMultiLineFormat(
} else if ("cancel_after_time_interval".equals(entry.getKey())
|| "cancelAfterTimeInterval".equals(entry.getKey())) {
searchRequest.setCancelAfterTimeInterval(nodeTimeValue(value, null));
} else if ("phase_took".equals(entry.getKey())) {
searchRequest.setPhaseTookQueryParamEnabled(SearchRequest.parseParamValue(value));
} else {
throw new IllegalArgumentException("key [" + entry.getKey() + "] is not supported in the metadata section");
}
Expand Down Expand Up @@ -374,6 +376,9 @@ 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());
}
xContentBuilder.endObject();
}

Expand Down
12 changes: 11 additions & 1 deletion server/src/main/java/org/opensearch/action/search/SearchRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,16 @@ enum ParamValue {
UNSET
}

public static Boolean parseParamValue(Object str) {
if ("TRUE".equals(str)) {
return true;
} else if ("FALSE".equals(str)) {
return false;
} else {
return null;
}
}

/**
* Returns value of user-provided phase_took query parameter for this search request.
* Defaults to <code>false</code>.
Expand All @@ -643,7 +653,7 @@ public ParamValue isPhaseTookQueryParamEnabled() {
/**
* Sets value of phase_took query param if provided by user. Defaults to <code>null</code>.
*/
public void setPhaseTookQueryParamEnabled(boolean phaseTookQueryParamEnabled) {
public void setPhaseTookQueryParamEnabled(Boolean phaseTookQueryParamEnabled) {
this.phaseTookQueryParamEnabled = phaseTookQueryParamEnabled;
}

Expand Down
25 changes: 12 additions & 13 deletions server/src/main/java/org/opensearch/action/search/SearchResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ public SearchResponse(StreamInput in) throws IOException {
if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
phaseTook = new PhaseTook(in);
} else {
phaseTook = PhaseTook.NULL;
phaseTook = PhaseTook.EMPTY;
}
skippedShards = in.readVInt();
pointInTimeId = in.readOptionalString();
Expand All @@ -141,7 +141,7 @@ public SearchResponse(
successfulShards,
skippedShards,
tookInMillis,
SearchResponse.PhaseTook.NULL,
SearchResponse.PhaseTook.EMPTY,
shardFailures,
clusters,
null
Expand Down Expand Up @@ -326,7 +326,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.NULL) == false) {
if (phaseTook.equals(PhaseTook.EMPTY) == false) {
phaseTook.toXContent(builder, params);
}
builder.field(TIMED_OUT.getPreferredName(), isTimedOut());
Expand Down Expand Up @@ -368,7 +368,7 @@ public static SearchResponse innerFromXContent(XContentParser parser) throws IOE
Boolean terminatedEarly = null;
int numReducePhases = 1;
long tookInMillis = -1;
PhaseTook phaseTook = PhaseTook.NULL;
PhaseTook phaseTook = PhaseTook.EMPTY;
int successfulShards = -1;
int totalShards = -1;
int skippedShards = 0; // 0 for BWC
Expand Down Expand Up @@ -441,7 +441,7 @@ public static SearchResponse innerFromXContent(XContentParser parser) throws IOE
currentFieldName = parser.currentName();
} else if (token.isValue()) {
try {
Enum.valueOf(SearchPhaseName.class, currentFieldName);
SearchPhaseName.valueOf(currentFieldName.toUpperCase());
phaseTookMap.put(currentFieldName, parser.longValue());
} catch (final IllegalArgumentException ex) {
parser.skipChildren();
Expand Down Expand Up @@ -665,17 +665,18 @@ public String toString() {
* @opensearch.internal
*/
public static class PhaseTook implements ToXContentFragment, Writeable {
public static final PhaseTook NULL = new PhaseTook();
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> nullPhaseTookMap = new HashMap<>();
Map<String, Long> defaultPhaseTookMap = new HashMap<>();
for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
nullPhaseTookMap.put(searchPhaseName.getName(), (long) -1);
defaultPhaseTookMap.put(searchPhaseName.getName(), (long) -1);
}
this.phaseStatsMap = nullPhaseTookMap;
this.phaseStatsMap = defaultPhaseTookMap;
}

public PhaseTook(Map<String, Long> phaseStatsMap) {
Expand All @@ -688,9 +689,7 @@ private PhaseTook(StreamInput in) throws IOException {

@Override
public void writeTo(StreamOutput out) throws IOException {
for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
out.writeLong(phaseStatsMap.get(searchPhaseName.getName()));
}
out.writeMap(phaseStatsMap, StreamOutput::writeString, StreamOutput::writeLong);
}

@Override
Expand Down Expand Up @@ -747,7 +746,7 @@ static SearchResponse empty(Supplier<Long> tookInMillisSupplier, Clusters cluste
0,
0,
tookInMillisSupplier.get(),
PhaseTook.NULL,
PhaseTook.EMPTY,
ShardSearchFailure.EMPTY_ARRAY,
clusters,
null
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/opensearch/action/search/SearchScrollAsyncAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ protected final void sendResponse(
successfulOps.get(),
0,
buildTookInMillis(),
SearchResponse.PhaseTook.NULL,
SearchResponse.PhaseTook.EMPTY,
buildShardFailures(),
SearchResponse.Clusters.EMPTY,
null
Expand Down
26 changes: 16 additions & 10 deletions server/src/main/java/org/opensearch/action/search/TransportSearchAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ SearchResponse.PhaseTook getPhaseTook() {
}
return new SearchResponse.PhaseTook(phaseTookMap);
} else {
return SearchResponse.PhaseTook.NULL;
return SearchResponse.PhaseTook.EMPTY;
}
}

Expand All @@ -341,6 +341,10 @@ public void onPhaseEnd(SearchPhaseContext context) {

@Override
public void onPhaseFailure(SearchPhaseContext context) {}

public long getPhaseTookTime(SearchPhaseName searchPhaseName) {
return phaseStatsMap.get(searchPhaseName);
}
}

@Override
Expand Down Expand Up @@ -463,15 +467,8 @@ private void executeRequest(
relativeStartNanos,
System::nanoTime
);
// 2nd executeRequest(), called by 1st executeRequest()
final List<SearchRequestOperationsListener> searchListenersList = createSearchListenerList();

if (originalSearchRequest.isPhaseTookQueryParamEnabled() == SearchRequest.ParamValue.TRUE
|| (originalSearchRequest.isPhaseTookQueryParamEnabled() == SearchRequest.ParamValue.UNSET
&& clusterService.getClusterSettings().get(TransportSearchAction.SEARCH_PHASE_TOOK_ENABLED))) {
timeProvider.setPhaseTookEnabled(true);
searchListenersList.add(timeProvider);
}
final List<SearchRequestOperationsListener> searchListenersList = createSearchListenerList(originalSearchRequest, timeProvider);

final SearchRequestOperationsListener searchRequestOperationsListener;
if (!CollectionUtils.isEmpty(searchListenersList)) {
Expand Down Expand Up @@ -1201,11 +1198,20 @@ AbstractSearchAsyncAction<? extends SearchPhaseResult> asyncSearchAction(
);
}

private List<SearchRequestOperationsListener> createSearchListenerList() {
private List<SearchRequestOperationsListener> createSearchListenerList(SearchRequest searchRequest, SearchTimeProvider timeProvider) {
final List<SearchRequestOperationsListener> searchListenersList = new ArrayList<>();

if (isRequestStatsEnabled) {
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);
}

return searchListenersList;
}

Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/opensearch/action/search/CreatePitControllerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public void setupData() {
3,
0,
100,
SearchResponse.PhaseTook.NULL,
SearchResponse.PhaseTook.EMPTY,
ShardSearchFailure.EMPTY_ARRAY,
SearchResponse.Clusters.EMPTY,
pitId
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/opensearch/action/search/MockSearchPhaseContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ public void sendSearchResponse(InternalSearchResponse internalSearchResponse, At
numSuccess.get(),
0,
0,
SearchResponse.PhaseTook.NULL,
SearchResponse.PhaseTook.EMPTY,
failures.toArray(ShardSearchFailure.EMPTY_ARRAY),
SearchResponse.Clusters.EMPTY,
searchContextId
Expand Down
4 changes: 2 additions & 2 deletions server/src/test/java/org/opensearch/action/search/MultiSearchResponseTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ protected MultiSearchResponse createTestInstance() {
successfulShards,
skippedShards,
tookInMillis,
SearchResponse.PhaseTook.NULL,
SearchResponse.PhaseTook.EMPTY,
ShardSearchFailure.EMPTY_ARRAY,
clusters,
null
Expand Down Expand Up @@ -97,7 +97,7 @@ private static MultiSearchResponse createTestInstanceWithFailures() {
successfulShards,
skippedShards,
tookInMillis,
SearchResponse.PhaseTook.NULL,
SearchResponse.PhaseTook.EMPTY,
ShardSearchFailure.EMPTY_ARRAY,
clusters,
null
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/opensearch/action/search/SearchAsyncActionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.NULL, ShardSearchFailure.EMPTY_ARRAY, Clusters.EMPTY, null);
super(InternalSearchResponse.empty(), null, 0, 0, 0, 0L, PhaseTook.EMPTY, ShardSearchFailure.EMPTY_ARRAY, Clusters.EMPTY, null);
}
}

Expand Down
33 changes: 21 additions & 12 deletions server/src/test/java/org/opensearch/action/search/SearchResponseTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;

import static java.util.Collections.singletonMap;
Expand Down Expand Up @@ -152,13 +154,11 @@ public SearchResponse createTestItem(
Boolean terminatedEarly = randomBoolean() ? null : randomBoolean();
int numReducePhases = randomIntBetween(1, 10);
long tookInMillis = randomNonNegativeLong();
SearchResponse.PhaseTook phaseTook = new SearchResponse.PhaseTook(
randomNonNegativeLong(),
randomNonNegativeLong(),
randomNonNegativeLong(),
randomNonNegativeLong(),
randomNonNegativeLong()
);
Map<String, Long> phaseTookMap = new HashMap<>();
for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
phaseTookMap.put(searchPhaseName.getName(), randomNonNegativeLong());
}
SearchResponse.PhaseTook phaseTook = new SearchResponse.PhaseTook(phaseTookMap);
int totalShards = randomIntBetween(1, Integer.MAX_VALUE);
int successfulShards = randomIntBetween(0, totalShards);
int skippedShards = randomIntBetween(0, totalShards);
Expand Down Expand Up @@ -328,7 +328,7 @@ public void testToXContent() {
0,
0,
0,
SearchResponse.PhaseTook.NULL,
SearchResponse.PhaseTook.EMPTY,
ShardSearchFailure.EMPTY_ARRAY,
SearchResponse.Clusters.EMPTY,
null
Expand Down Expand Up @@ -362,6 +362,14 @@ public void testToXContent() {
assertEquals(1, searchExtBuilders.size());
}
{
Map<String, Long> phaseTookMap = new HashMap<>();
for (SearchPhaseName searchPhaseName : SearchPhaseName.values()) {
phaseTookMap.put(searchPhaseName.getName(), 0L);
}
phaseTookMap.put(SearchPhaseName.QUERY.getName(), 50L);
phaseTookMap.put(SearchPhaseName.FETCH.getName(), 25L);
phaseTookMap.put(SearchPhaseName.EXPAND.getName(), 30L);
SearchResponse.PhaseTook phaseTook = new SearchResponse.PhaseTook(phaseTookMap);
SearchResponse response = new SearchResponse(
new InternalSearchResponse(
new SearchHits(hits, new TotalHits(100, TotalHits.Relation.EQUAL_TO), 1.5f),
Expand All @@ -377,7 +385,7 @@ public void testToXContent() {
0,
0,
0,
new SearchResponse.PhaseTook(0, 0, 50, 25, 0),
phaseTook,
ShardSearchFailure.EMPTY_ARRAY,
new SearchResponse.Clusters(5, 3, 2),
null
Expand All @@ -388,11 +396,12 @@ public void testToXContent() {
expectedString.append("\"took\":0,");
expectedString.append("\"phase_took\":");
{
expectedString.append("{\"dfs_prequery\":0,");
expectedString.append("\"can_match\":0,");
expectedString.append("{\"dfs_pre_query\":0,");
expectedString.append("\"query\":50,");
expectedString.append("\"fetch\":25,");
expectedString.append("\"expand_search\":0},");
expectedString.append("\"dfs_query\":0,");
expectedString.append("\"expand\":30,");
expectedString.append("\"can_match\":0},");
}
expectedString.append("\"timed_out\":false,");
expectedString.append("\"_shards\":");
Expand Down
Loading

0 comments on commit 3572783

Please sign in to comment.