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

Field-caps should read fields from up-to-dated shards #105153

Merged
merged 4 commits into from
Feb 5, 2024

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Feb 5, 2024

I have seen scenarios in which field-caps return information from outdated shards. While this is probably acceptable for most cases, ESQL query planning relies on field-caps, potentially leading to missing data. The reason for this is that we don't check readAllowed when not acquiring a searcher for cases without a filter. I don't expect too much penalty in terms of performance with this change, but it helps avoid a subtle issue for ESQL.

Closes #104809

@dnhatn dnhatn added >bug :Search/Search Search-related issues that do not fall into other categories :Analytics/ES|QL AKA ESQL v8.13.0 v8.12.2 labels Feb 5, 2024
@elasticsearchmachine elasticsearchmachine added Team:Search Meta label for search team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Feb 5, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @dnhatn, I've created a changelog YAML for you.

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

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

@dnhatn dnhatn removed the :Analytics/ES|QL AKA ESQL label Feb 5, 2024
@dnhatn
Copy link
Member Author

dnhatn commented Feb 5, 2024

test this please

@elasticsearchmachine elasticsearchmachine removed the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 5, 2024
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM in principle, we should look out for performance impact in the many shards benchmarks though I think. We are adding another volatile read here :)

@dnhatn
Copy link
Member Author

dnhatn commented Feb 5, 2024

LGTM in principle, we should look out for performance impact in the many shards benchmarks though I think. We are adding another volatile read here :)

Thanks Armin. I will keep an eye on the benchmark.

@dnhatn dnhatn merged commit 40e0f1f into elastic:main Feb 5, 2024
15 checks passed
@dnhatn dnhatn deleted the field-caps branch February 5, 2024 23:34
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.12

dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Feb 5, 2024
I have seen scenarios in which field-caps return information from 
outdated shards. While this is probably acceptable for most cases, ESQL
query planning relies on field-caps, potentially leading to missing
data. The reason for this is that we don't check readAllowed when not
acquiring a searcher for cases without a filter. I don't expect too much
penalty in terms of performance with this change, but it helps avoid a
subtle issue for ESQL.

Closes elastic#104809
elasticsearchmachine pushed a commit that referenced this pull request Feb 6, 2024
)

I have seen scenarios in which field-caps return information from 
outdated shards. While this is probably acceptable for most cases, ESQL
query planning relies on field-caps, potentially leading to missing
data. The reason for this is that we don't check readAllowed when not
acquiring a searcher for cases without a filter. I don't expect too much
penalty in terms of performance with this change, but it helps avoid a
subtle issue for ESQL.

Closes #104809
@dnhatn
Copy link
Member Author

dnhatn commented Feb 9, 2024

@original-brownbear I checked the field-caps benchmark, and there have been no regressions.

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

Successfully merging this pull request may close these issues.

[CI] EsqlClientYamlIT test {p0=esql/30_types/_source} failing
3 participants