-
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
Changes from 3 commits
9bfc687
4714df1
ba24a24
687e383
e01c115
9ef6131
06b880b
b3ff223
77807c4
e540ac0
c7a1ad4
23a1c66
c54e7ca
9dc4a25
9ae4e6a
ea27c07
28dc9fd
50065a1
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 |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
|
|
||
| import java.io.IOException; | ||
| import java.util.List; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import java.util.function.Predicate; | ||
|
|
||
|
|
@@ -37,11 +38,11 @@ public class RestSearchTemplateAction extends BaseRestHandler { | |
| private static final Set<String> RESPONSE_PARAMS = Set.of(TYPED_KEYS_PARAM, RestSearchAction.TOTAL_HITS_AS_INT_PARAM); | ||
|
|
||
| private final Predicate<NodeFeature> clusterSupportsFeature; | ||
| private final Settings settings; | ||
| private final boolean crossProjectEnabled; | ||
|
|
||
| public RestSearchTemplateAction(Predicate<NodeFeature> clusterSupportsFeature, Settings settings) { | ||
| this.clusterSupportsFeature = clusterSupportsFeature; | ||
| this.settings = settings; | ||
| this.crossProjectEnabled = settings != null && settings.getAsBoolean("serverless.cross_project.enabled", false); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -61,7 +62,7 @@ public String getName() { | |
|
|
||
| @Override | ||
| public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { | ||
| if (settings != null && settings.getAsBoolean("serverless.cross_project.enabled", false)) { | ||
| if (crossProjectEnabled) { | ||
| // accept but drop project_routing param until fully supported | ||
| request.param("project_routing"); | ||
| } | ||
|
|
@@ -73,7 +74,9 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client | |
| request, | ||
| null, | ||
| clusterSupportsFeature, | ||
| size -> searchRequest.source().size(size) | ||
| size -> searchRequest.source().size(size), | ||
| null, | ||
| Optional.of(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. 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
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. 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
Endpoints that aren't explicitly consuming MRT are defaulting to Own parser = the parsing happens within the respective
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. Awesome. Thanks. Excellent analysis. 🥇 |
||
| ); | ||
|
|
||
| // Creates the search template request | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,7 @@ | |
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import java.util.function.IntConsumer; | ||
| import java.util.function.Predicate; | ||
|
|
@@ -68,13 +69,11 @@ public class RestSearchAction extends BaseRestHandler { | |
|
|
||
| private final SearchUsageHolder searchUsageHolder; | ||
| private final Predicate<NodeFeature> clusterSupportsFeature; | ||
| private final Settings settings; | ||
| private final CrossProjectModeDecider crossProjectModeDecider; | ||
|
|
||
| public RestSearchAction(SearchUsageHolder searchUsageHolder, Predicate<NodeFeature> clusterSupportsFeature, Settings settings) { | ||
| this.searchUsageHolder = searchUsageHolder; | ||
| this.clusterSupportsFeature = clusterSupportsFeature; | ||
| this.settings = settings; | ||
| this.crossProjectModeDecider = new CrossProjectModeDecider(settings); | ||
| } | ||
|
|
||
|
|
@@ -134,7 +133,7 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC | |
| clusterSupportsFeature, | ||
| setSize, | ||
| searchUsageHolder, | ||
| crossProjectEnabled | ||
| Optional.of(crossProjectEnabled) | ||
| ) | ||
| ); | ||
|
|
||
|
|
@@ -146,7 +145,9 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC | |
|
|
||
| /** | ||
| * Parses the rest request on top of the SearchRequest, preserving values that are not overridden by the rest request. | ||
| * | ||
| * The endpoint calling this method is treated as if it does not support Cross Project Search (CPS). In case it supports | ||
| * CPS, it should call in the other appropriate overload and pass in the CPS state explicitly either via Optional.of(true) | ||
| * or Optional.of(false). | ||
| * @param searchRequest the search request that will hold what gets parsed | ||
| * @param request the rest request to read from | ||
| * @param requestContentParser body of the request to read. This method does not attempt to read the body from the {@code request} | ||
|
|
@@ -172,7 +173,15 @@ public static void parseSearchRequest( | |
| IntConsumer setSize, | ||
| @Nullable SearchUsageHolder searchUsageHolder | ||
| ) throws IOException { | ||
| parseSearchRequest(searchRequest, request, requestContentParser, clusterSupportsFeature, setSize, searchUsageHolder, false); | ||
| parseSearchRequest( | ||
| searchRequest, | ||
| request, | ||
| requestContentParser, | ||
| clusterSupportsFeature, | ||
| setSize, | ||
| searchUsageHolder, | ||
| Optional.empty() | ||
|
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. Previously, this overload passed in |
||
| ); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -185,7 +194,10 @@ public static void parseSearchRequest( | |
| * @param clusterSupportsFeature used to check if certain features are available in this cluster | ||
| * @param setSize how the size url parameter is handled. {@code udpate_by_query} and regular search differ here. | ||
| * @param searchUsageHolder the holder of search usage stats | ||
| * @param crossProjectEnabled whether serverless.cross_project.enabled is set to true | ||
| * @param crossProjectEnabled Specifies the state of Cross Project Search (CPS) for the endpoint that's calling this method. | ||
| * Optional.of(true) - signifies that the endpoint supports CPS, | ||
| * Optional.of(false) - signifies that the endpoint supports CPS but CPS is disabled, and, | ||
| * Optional.empty() - signifies that the endpoint does not support CPS. | ||
| */ | ||
| public static void parseSearchRequest( | ||
| SearchRequest searchRequest, | ||
|
|
@@ -194,7 +206,7 @@ public static void parseSearchRequest( | |
| Predicate<NodeFeature> clusterSupportsFeature, | ||
| IntConsumer setSize, | ||
| @Nullable SearchUsageHolder searchUsageHolder, | ||
| boolean crossProjectEnabled | ||
| Optional<Boolean> crossProjectEnabled | ||
| ) throws IOException { | ||
| if (searchRequest.source() == null) { | ||
| searchRequest.source(new SearchSourceBuilder()); | ||
|
|
@@ -205,7 +217,7 @@ public static void parseSearchRequest( | |
| * We only do it if in a Cross Project Environment, though, because outside it, such details are not | ||
| * expected and valid. | ||
| */ | ||
| SearchRequest searchRequestForParsing = crossProjectEnabled ? searchRequest : null; | ||
| SearchRequest searchRequestForParsing = crossProjectEnabled.orElse(false) ? searchRequest : null; | ||
| if (requestContentParser != null) { | ||
| if (searchUsageHolder == null) { | ||
| searchRequest.source().parseXContent(searchRequestForParsing, requestContentParser, true, clusterSupportsFeature); | ||
|
|
@@ -250,7 +262,7 @@ public static void parseSearchRequest( | |
| searchRequest.routing(request.param("routing")); | ||
| searchRequest.preference(request.param("preference")); | ||
| IndicesOptions indicesOptions = IndicesOptions.fromRequest(request, searchRequest.indicesOptions()); | ||
| if (crossProjectEnabled && searchRequest.allowsCrossProject() && searchRequest.pointInTimeBuilder() == null) { | ||
| if (crossProjectEnabled.orElse(false) && searchRequest.allowsCrossProject() && searchRequest.pointInTimeBuilder() == null) { | ||
| indicesOptions = IndicesOptions.builder(indicesOptions) | ||
| .crossProjectModeOptions(new IndicesOptions.CrossProjectModeOptions(true)) | ||
| .build(); | ||
|
|
@@ -262,9 +274,30 @@ public static void parseSearchRequest( | |
| if (searchRequest.pointInTimeBuilder() != null) { | ||
| preparePointInTime(searchRequest, request); | ||
| } else { | ||
| searchRequest.setCcsMinimizeRoundtrips( | ||
| request.paramAsBoolean("ccs_minimize_roundtrips", searchRequest.isCcsMinimizeRoundtrips()) | ||
| ); | ||
| if (crossProjectEnabled.orElse(false)) { | ||
| /* | ||
| * MRT should not be settable by the user when in Cross Project Search environment. | ||
| * Only _async_search uses MRT=false. However, in RestSubmitAsyncSearchAction, we | ||
| * already, explicitly, and directly call in `SearchRequest#setCcsMinimizeRoundtrips()` | ||
| * to set it to true. Although other searches that utilise SearchRequest-s do not call | ||
| * this method, SearchRequest, by default, sets MRT to true when it is instantiated. | ||
| * This way, all searches pivot to MRT=true for CPS. | ||
| * | ||
| * Since users will anyway see a banner via Kibana that setting MRT in Serverless has no | ||
| * effect, we can safely drop it. | ||
| */ | ||
| if (request.hasParam("ccs_minimize_roundtrips")) { | ||
| request.param("ccs_minimize_roundtrips"); | ||
| } | ||
| } else { | ||
| /* | ||
| * Either we're not in a Cross Project Search environment or the endpoint isn't compatible with it. Parse what's in the | ||
| * request. | ||
| */ | ||
| searchRequest.setCcsMinimizeRoundtrips( | ||
| request.paramAsBoolean("ccs_minimize_roundtrips", searchRequest.isCcsMinimizeRoundtrips()) | ||
| ); | ||
| } | ||
| } | ||
| if (request.paramAsBoolean("force_synthetic_source", false)) { | ||
| searchRequest.setForceSyntheticSource(true); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.