-
Notifications
You must be signed in to change notification settings - Fork 25.6k
MRT should default to true for CPS searches
#138105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9bfc687
4714df1
ba24a24
687e383
e01c115
9ef6131
06b880b
b3ff223
77807c4
e540ac0
c7a1ad4
23a1c66
c54e7ca
9dc4a25
9ae4e6a
ea27c07
28dc9fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| pr: 138105 | ||
| summary: MRT should default to `true` for CPS searches | ||
| area: CCS | ||
| type: enhancement | ||
| issues: [] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,9 @@ | |
| import org.elasticsearch.common.bytes.BytesReference; | ||
| import org.elasticsearch.common.io.stream.StreamInput; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
| import org.elasticsearch.common.logging.HeaderWarning; | ||
| import org.elasticsearch.common.xcontent.XContentHelper; | ||
| import org.elasticsearch.rest.action.search.SearchParamsParser; | ||
| import org.elasticsearch.tasks.CancellableTask; | ||
| import org.elasticsearch.tasks.Task; | ||
| import org.elasticsearch.tasks.TaskId; | ||
|
|
@@ -36,6 +38,7 @@ | |
| import java.util.Locale; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Optional; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import static org.elasticsearch.action.ValidateActions.addValidationError; | ||
|
|
@@ -167,7 +170,12 @@ public static void readMultiLineFormat( | |
| String routing, | ||
| String searchType, | ||
| Boolean ccsMinimizeRoundtrips, | ||
| boolean allowExplicitIndex | ||
| boolean allowExplicitIndex, | ||
| /* | ||
| * Refer to RestSearchAction#parseSearchRequest()'s JavaDoc to understand why this is an Optional | ||
| * and what its values mean with respect to an endpoint's Cross Project Search status/support. | ||
| */ | ||
| Optional<Boolean> crossProjectEnabled | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional suggestion: Add javadoc here what Optional.TRUE, Optional.FALSE and Optional.empty mean. That's not going to be obvious. Or if you feel like you have to document that 20 times, put a note here on where to see the javadoc for that?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| ) throws IOException { | ||
| readMultiLineFormat( | ||
| xContent, | ||
|
|
@@ -180,7 +188,8 @@ public static void readMultiLineFormat( | |
| searchType, | ||
| ccsMinimizeRoundtrips, | ||
| allowExplicitIndex, | ||
| (s, o, r) -> false | ||
| (s, o, r) -> false, | ||
| crossProjectEnabled | ||
| ); | ||
|
|
||
| } | ||
|
|
@@ -196,10 +205,16 @@ public static void readMultiLineFormat( | |
| String searchType, | ||
| Boolean ccsMinimizeRoundtrips, | ||
| boolean allowExplicitIndex, | ||
| TriFunction<String, Object, SearchRequest, Boolean> extraParamParser | ||
| TriFunction<String, Object, SearchRequest, Boolean> extraParamParser, | ||
| /* | ||
| * Refer to RestSearchAction#parseSearchRequest()'s JavaDoc to understand why this is an Optional | ||
| * and what its values mean with respect to an endpoint's Cross Project Search status/support. | ||
| */ | ||
| Optional<Boolean> crossProjectEnabled | ||
| ) throws IOException { | ||
| int from = 0; | ||
| byte marker = xContent.bulkSeparator(); | ||
| boolean warnedMrtForCps = false; | ||
| while (true) { | ||
| int nextMarker = findNextMarker(marker, from, data); | ||
| if (nextMarker == -1) { | ||
|
|
@@ -219,6 +234,11 @@ public static void readMultiLineFormat( | |
| if (searchType != null) { | ||
| searchRequest.searchType(searchType); | ||
| } | ||
| /* | ||
| * This `ccsMinimizeRoundtrips` refers to the value specified as the query parameter and is extracted in | ||
| * `RestMultiSearchAction#parseMultiLineRequest()`. If in a Cross Project Search environment, it is | ||
| * guaranteed to be `true`. Otherwise, its value is whatever that the user is provided. | ||
| */ | ||
| if (ccsMinimizeRoundtrips != null) { | ||
| searchRequest.setCcsMinimizeRoundtrips(ccsMinimizeRoundtrips); | ||
| } | ||
|
|
@@ -250,7 +270,11 @@ public static void readMultiLineFormat( | |
| } else if ("search_type".equals(entry.getKey()) || "searchType".equals(entry.getKey())) { | ||
| searchRequest.searchType(nodeStringValue(value, null)); | ||
| } else if ("ccs_minimize_roundtrips".equals(entry.getKey()) || "ccsMinimizeRoundtrips".equals(entry.getKey())) { | ||
| searchRequest.setCcsMinimizeRoundtrips(nodeBooleanValue(value)); | ||
| searchRequest.setCcsMinimizeRoundtrips(crossProjectEnabled.orElse(false) || nodeBooleanValue(value)); | ||
| if (crossProjectEnabled.orElse(false) && warnedMrtForCps == false) { | ||
| HeaderWarning.addWarning(SearchParamsParser.MRT_SET_IN_CPS_WARN); | ||
| warnedMrtForCps = true; | ||
| } | ||
| } else if ("request_cache".equals(entry.getKey()) || "requestCache".equals(entry.getKey())) { | ||
| searchRequest.requestCache(nodeBooleanValue(value, entry.getKey())); | ||
| } else if ("preference".equals(entry.getKey())) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worries me that the only other RestAction (besides the obvious ones of RestSearchAction and the async RestSearchAction) you had to change was this one.
But there are other several Rest actions that converge on _search that are (or will be) also CPS enabled:
We should track down whether any of those allow setting
ccs_minimize_roundtripsand if so, if additional code changes are needed.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endpoints you've listed have their own dedicated parsing strategies that are lenient wrt the query parameters being passed, and there's some inconsistency between our public-facing docs and how the endpoints work. For example, for some of them, you can pass MRT, but it's not consumed. Here's why I only had to touch only a few
Rest*Action-s:_eqland_msearch/templateaccept MRT and have their own dedicated parser. I can accommodate these changes there._sqldoes not appear to accept MRT and has its own parser._mvtdoes not appear to accept MRT and has its own parser.{index}/_countdoes not appear to accept MRT and has its own simple parser._cat/countdoes not appear to accept MRT and has its own simple parser.Endpoints that aren't explicitly consuming MRT are defaulting to
true.Own parser = the parsing happens within the respective
Rest*Actionwith info manually being extracted viaRestRequest#param()and passed to the respective*Request-s.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Thanks. Excellent analysis. 🥇