-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Don't overwrite sync strategy in xpack #75556
Conversation
Pinging @elastic/kibana-app-arch (Team:AppArch) |
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.
I understand the impetus behind this PR but I worry that in the current implementation, setDefaultSearchStrategy
will seem like something plugins will want to use, when in fact I think it would make more sense to have it under the enhancements pattern because it really is internal to the data plugin. (Imagine if another plugin set the default search strategy to a custom strategy... then it would break all of our dashboards, etc.). Thoughts?
@@ -6,6 +6,8 @@ | |||
|
|||
import { IEsSearchRequest, ISearchRequestParams } from '../../../../../src/plugins/data/common'; | |||
|
|||
export const ENHANCED_ES_SEARCH_STRATEGY = 'ese'; |
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.
Can we give this a more meaningful name like enhanced_es
or something?
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.
I wanted to keep it shorter, as this is what gets sent in the URL.
I think keeping it short is clearer, and we use the constant name elsewhere anyway.
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.
And I did change it to use the enhance
pattern. I agree it makes more sense this way.
@elasticmachine merge upstream |
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
page load bundle size
History
To update your PR or re-run it, just comment with: |
* Don't override sync strategy in XPACK * search name * docs * mock * Use enhancement pattern Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* Don't override sync strategy in XPACK * search name * docs * mock * Use enhancement pattern Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
Previously, in x-pack, we had overriden the synchronous search strategy with an async one.
This PR keeps both strategies, changing the default only, to allow solutions to use the synchronous strategy in OSS, if needed.
Needed for #75115
Checklist
Delete any items that are not applicable to this PR.
For maintainers