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.

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

Disable get API on legacy indices #86594

Merged
merged 1 commit into from
May 10, 2022
Merged

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented May 10, 2022

The get API relies under the hood on accessing postings to lookup the _id and retrieve the corresponding document. Guaranteeing this access via postings is not something we would like to guarantee on archive indices. While we are adding "text field support" for archive indices, we reserve the flexibility to eventually swap that out with a "runtime-text field" variant, and hence only provide those capabilities that can be emulated via a runtime field. Doing the same for "get" would mean doing a full scan of the index (using stored fields), which is counterintuitive to what the get API is meant to be used for (quick lookup of document). We would therefore rather not have the API accessible on archive indices.

Relates #81210

@ywelsch ywelsch mentioned this pull request May 10, 2022
32 tasks
@ywelsch ywelsch marked this pull request as ready for review May 10, 2022 09:25
@ywelsch ywelsch added >non-issue :Search/Search Search-related issues that do not fall into other categories labels May 10, 2022
@elasticmachine elasticmachine added the Team:Search Meta label for search team label May 10, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@ywelsch ywelsch requested review from romseygeek and dnhatn May 10, 2022 09:26
@romseygeek
Copy link
Contributor

Can you add something to the description about why we want to do this? The code change looks correct to me but I don't understand the motivation for it yet :)

@ywelsch
Copy link
Contributor Author

ywelsch commented May 10, 2022

Can you add something to the description about why we want to do this? The code change looks correct to me but I don't understand the motivation for it yet :)

Of course! I've updated the PR description.

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @ywelsch

@ywelsch ywelsch merged commit cf2fcae into elastic:master May 10, 2022
@ywelsch
Copy link
Contributor Author

ywelsch commented May 10, 2022

Thanks @romseygeek!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants