Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[data.search] Clean up code around ES preference usage #204076

Merged
merged 9 commits into from
Jan 6, 2025
Merged

[data.search] Clean up code around ES preference usage #204076

merged 9 commits into from
Jan 6, 2025

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Dec 12, 2024 •
edited by kibanamachine
Loading

Summary

While refreshing my memory on how we are using the Elasticsearch preference parameter, I noticed we had a method getEsPreference that was exported but not used. We then had an additional method getPreference that was being used to actually set the parameter. I consolidated these into one place and moved the tests so that we can utilize it when ES|QL supports this parameter.

Checklist

@lukasolson lukasolson added Feature:Search Querying infrastructure in Kibana Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 12, 2024
@lukasolson lukasolson self-assigned this Dec 12, 2024
@lukasolson lukasolson added the release_note:skip Skip the PR/issue when compiling release notes label Dec 12, 2024
@lukasolson lukasolson marked this pull request as ready for review December 17, 2024 15:30
@lukasolson lukasolson requested review from a team as code owners December 17, 2024 15:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

markov00
markov00 approved these changes Dec 19, 2024
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise it looks good to me.

src/plugins/data/common/search/search_source/fetch/get_search_params.ts Outdated Show resolved Hide resolved
davismcphee
davismcphee approved these changes Jan 3, 2025
View reviewed changes
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-only review, LGTM 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 512 510 -2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
data 2593 2592 -1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 418.8KB 418.7KB -91.0B
Unknown metric groups

API count

id before after diff
data 3208 3207 -1

History

cc @lukasolson

@lukasolson lukasolson merged commit df3665f into elastic:main Jan 6, 2025
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12641084691

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 6, 2025
@lukasolson
[data.search] Clean up code around ES preference usage (elastic#204076)
39b81f4 
## Summary

While refreshing my memory on how we are using the Elasticsearch
`preference` parameter, I noticed we had a method `getEsPreference` that
was exported but not used. We then had an additional method
`getPreference` that was being used to actually set the parameter. I
consolidated these into one place and moved the tests so that we can
utilize it when ES|QL supports this parameter.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Marco Vettorello <[email protected]>
(cherry picked from commit df3665f)
@kibanamachine kibanamachine mentioned this pull request Jan 6, 2025
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jan 7, 2025
@kibanamachine @lukasolson
[8.x] [data.search] Clean up code around ES preference usage (#204076) (
b74ac18 
#205661)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[data.search] Clean up code around ES preference usage
(#204076)](#204076)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Lukas
Olson","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-01-06T22:02:48Z","message":"[data.search]
Clean up code around ES preference usage (#204076)\n\n##
Summary\r\n\r\nWhile refreshing my memory on how we are using the
Elasticsearch\r\n`preference` parameter, I noticed we had a method
`getEsPreference` that\r\nwas exported but not used. We then had an
additional method\r\n`getPreference` that was being used to actually set
the parameter. I\r\nconsolidated these into one place and moved the
tests so that we can\r\nutilize it when ES|QL supports this
parameter.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Vettorello
<[email protected]>","sha":"df3665fbf59295dfbd664dfc280658b5232498d9","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Search","release_note:skip","v9.0.0","Team:DataDiscovery","backport:prev-minor"],"title":"[data.search]
Clean up code around ES preference
usage","number":204076,"url":"https://github.com/elastic/kibana/pull/204076","mergeCommit":{"message":"[data.search]
Clean up code around ES preference usage (#204076)\n\n##
Summary\r\n\r\nWhile refreshing my memory on how we are using the
Elasticsearch\r\n`preference` parameter, I noticed we had a method
`getEsPreference` that\r\nwas exported but not used. We then had an
additional method\r\n`getPreference` that was being used to actually set
the parameter. I\r\nconsolidated these into one place and moved the
tests so that we can\r\nutilize it when ES|QL supports this
parameter.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Vettorello
<[email protected]>","sha":"df3665fbf59295dfbd664dfc280658b5232498d9"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/204076","number":204076,"mergeCommit":{"message":"[data.search]
Clean up code around ES preference usage (#204076)\n\n##
Summary\r\n\r\nWhile refreshing my memory on how we are using the
Elasticsearch\r\n`preference` parameter, I noticed we had a method
`getEsPreference` that\r\nwas exported but not used. We then had an
additional method\r\n`getPreference` that was being used to actually set
the parameter. I\r\nconsolidated these into one place and moved the
tests so that we can\r\nutilize it when ES|QL supports this
parameter.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Marco Vettorello
<[email protected]>","sha":"df3665fbf59295dfbd664dfc280658b5232498d9"}}]}]
BACKPORT-->

Co-authored-by: Lukas Olson <[email protected]>
kowalczyk-krzysztof pushed a commit to kowalczyk-krzysztof/kibana that referenced this pull request Jan 7, 2025
@lukasolson @markov00 @kowalczyk-krzysztof
[data.search] Clean up code around ES preference usage (elastic#204076)
dd04900 
## Summary

While refreshing my memory on how we are using the Elasticsearch
`preference` parameter, I noticed we had a method `getEsPreference` that
was exported but not used. We then had an additional method
`getPreference` that was being used to actually set the parameter. I
consolidated these into one place and moved the tests so that we can
utilize it when ES|QL supports this parameter.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Marco Vettorello <[email protected]>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Jan 13, 2025
@lukasolson @markov00 @CAWilson94
[data.search] Clean up code around ES preference usage (elastic#204076)
b5d8d28 
## Summary

While refreshing my memory on how we are using the Elasticsearch
`preference` parameter, I noticed we had a method `getEsPreference` that
was exported but not used. We then had an additional method
`getPreference` that was being used to actually set the parameter. I
consolidated these into one place and moved the tests so that we can
utilize it when ES|QL supports this parameter.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Marco Vettorello <[email protected]>
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
@lukasolson @markov00 @viduni94
[data.search] Clean up code around ES preference usage (elastic#204076)
2daa3e5 
## Summary

While refreshing my memory on how we are using the Elasticsearch
`preference` parameter, I noticed we had a method `getEsPreference` that
was exported but not used. We then had an additional method
`getPreference` that was being used to actually set the parameter. I
consolidated these into one place and moved the tests so that we can
utilize it when ES|QL supports this parameter.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Marco Vettorello <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davismcphee davismcphee davismcphee approved these changes

@markov00 markov00 markov00 approved these changes

Assignees

@lukasolson lukasolson

Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.18.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lukasolson @elasticmachine @kibanamachine @markov00 @davismcphee