-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ES|QL: Add project routing to request body and configuration #138580
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
ES|QL: Add project routing to request body and configuration #138580
Conversation
| clusterSettings.timeseriesResultTruncationMaxSize(), | ||
| clusterSettings.timeseriesResultTruncationDefaultSize() | ||
| clusterSettings.timeseriesResultTruncationDefaultSize(), | ||
| statement.setting(QuerySettings.PROJECT_ROUTING) |
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.
If I understand correctly we should also be able to take it from other places (request field and cluster state, I do not remember the order of preference)?
Should we start implementing it as part of this change?
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.
Yeah, I think it makes sense to start adding support for this in the scope of this PR
I'm on it (ref. ES-13390)
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.
Done, now we have project_routing also in the request body.
Users will be able to set both, and SET parameter will have precendence.
I also added some basic validation, to prevent usage in contexts where CPS is not enabled
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| if (projectRouting == null) { | ||
| projectRouting = request.projectRouting(); | ||
| } | ||
| if (projectRouting != null && executionInfo.isCrossClusterSearch() == false) { |
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.
executionInfo.isCrossClusterSearch() == false is misleading.
We are populating project routing before FC call (as intended), but executionInfo.clusterInfo that is backing this condition is populated after. I believe it is only passing because executionInfo.isCrossClusterSearch() happens to be false for empty/non-initialized data structure.
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.
You're absolutely right, I don't know why I picked executionInfo, I should have used CrossProjectModeDecider.
Let me fix it
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.
Fixed
Adding project routing to the request body and to the configuration.
It will be used for index resolution.
This also adds validation, so that this option can only be set in a context where CPS is enabled.
The existing SET parameter is still supported, and will have precedence on the request body setting, ie. in
the resulting value for project routing will be "foo".