Skip to content

Return an empty suggestion when suggest phase times out #122575

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

Merged
merged 10 commits into from
Feb 14, 2025

Conversation

javanna
Copy link
Member

@javanna javanna commented Feb 14, 2025

We recently fixed timeout handling in the suggest phase. A test failure on SearchTimeoutIT surfaced an issue with the current approach. In case partial results are allowed, it may happen that some shards time out while executing the suggest phase and some don't.

SearchPhaseController assumes that if one shard has suggest results, all of the other shards will have suggest results too. We could address that assertion and check is the search timed out, instead this commit changes timeout handling in the suggest phase to return an empty suggestion instead of null. This seems appropriate in terms of providing some results and makes the assertion about non null suggestions in SearchPhaseController happy.

Relates to #122357

Closes #122548

@javanna javanna added >bug :Search Relevance/Suggesters "Did you mean" and suggestions as you type auto-backport Automatically create backport pull requests when merged :Search Foundations/Search Catch all for Search Foundations v8.18.1 v8.19.0 v9.1.0 labels Feb 14, 2025
@elasticsearchmachine elasticsearchmachine added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch labels Feb 14, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @javanna, I've updated the changelog YAML for you.

javanna and others added 10 commits February 14, 2025 10:00
We recently fixed timeout handling in the suggest phase. A test failure on SearchTimeoutIT
surfaced an issue with the current approach. In case partial results are allowed, it may
happen that some shards time out while executing the suggest phase and some don't.

SearchPhaseController assumes that if one shard has suggest results, all of the other shards
will have suggest results too. We could address that assertion and check is the search
timed out, instead this commit changes timeout handling in the suggest phase to return
an empty suggestion instead of null. This seems appropriate in terms of providing some
results and makes the assertion about non null suggestions in SearchPhaseController happy.

Relates to elastic#122357
Copy link
Member

@piergm piergm left a comment

Choose a reason for hiding this comment

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

LGTM!

@javanna javanna merged commit 66d18d0 into elastic:main Feb 14, 2025
16 of 17 checks passed
@javanna javanna deleted the fix/suggest_timeout_empty_suggestion branch February 14, 2025 14:28
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.18 Commit could not be cherrypicked due to conflicts
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 122575

@javanna javanna added the v9.0.0 label Feb 14, 2025
javanna added a commit to javanna/elasticsearch that referenced this pull request Feb 15, 2025
We recently fixed timeout handling in the suggest phase. A test failure on SearchTimeoutIT surfaced an issue with the current approach. In case partial results are allowed, it may happen that some shards time out while executing the suggest phase and some don't.

SearchPhaseController assumes that if one shard has suggest results, all of the other shards will have suggest results too. We could address that assertion and check is the search timed out, instead this commit changes timeout handling in the suggest phase to return an empty suggestion instead of null. This seems appropriate in terms of providing some results and makes the assertion about non null suggestions in SearchPhaseController happy.

Relates to elastic#122357

Closes elastic#122548
javanna added a commit to javanna/elasticsearch that referenced this pull request Feb 15, 2025
We recently fixed timeout handling in the suggest phase. A test failure on SearchTimeoutIT surfaced an issue with the current approach. In case partial results are allowed, it may happen that some shards time out while executing the suggest phase and some don't.

SearchPhaseController assumes that if one shard has suggest results, all of the other shards will have suggest results too. We could address that assertion and check is the search timed out, instead this commit changes timeout handling in the suggest phase to return an empty suggestion instead of null. This seems appropriate in terms of providing some results and makes the assertion about non null suggestions in SearchPhaseController happy.

Relates to elastic#122357

Closes elastic#122548
elasticsearchmachine pushed a commit that referenced this pull request Feb 15, 2025
…22676)

We recently fixed timeout handling in the suggest phase. A test failure on SearchTimeoutIT surfaced an issue with the current approach. In case partial results are allowed, it may happen that some shards time out while executing the suggest phase and some don't.

SearchPhaseController assumes that if one shard has suggest results, all of the other shards will have suggest results too. We could address that assertion and check is the search timed out, instead this commit changes timeout handling in the suggest phase to return an empty suggestion instead of null. This seems appropriate in terms of providing some results and makes the assertion about non null suggestions in SearchPhaseController happy.

Relates to #122357

Closes #122548
elasticsearchmachine pushed a commit that referenced this pull request Feb 15, 2025
…22675)

We recently fixed timeout handling in the suggest phase. A test failure on SearchTimeoutIT surfaced an issue with the current approach. In case partial results are allowed, it may happen that some shards time out while executing the suggest phase and some don't.

SearchPhaseController assumes that if one shard has suggest results, all of the other shards will have suggest results too. We could address that assertion and check is the search timed out, instead this commit changes timeout handling in the suggest phase to return an empty suggestion instead of null. This seems appropriate in terms of providing some results and makes the assertion about non null suggestions in SearchPhaseController happy.

Relates to #122357

Closes #122548
elasticsearchmachine pushed a commit that referenced this pull request Feb 15, 2025
…22677)

We recently fixed timeout handling in the suggest phase. A test failure on SearchTimeoutIT surfaced an issue with the current approach. In case partial results are allowed, it may happen that some shards time out while executing the suggest phase and some don't.

SearchPhaseController assumes that if one shard has suggest results, all of the other shards will have suggest results too. We could address that assertion and check is the search timed out, instead this commit changes timeout handling in the suggest phase to return an empty suggestion instead of null. This seems appropriate in terms of providing some results and makes the assertion about non null suggestions in SearchPhaseController happy.

Relates to #122357

Closes #122548
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged >bug :Search Foundations/Search Catch all for Search Foundations :Search Relevance/Suggesters "Did you mean" and suggestions as you type Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch Team:Search Relevance Meta label for the Search Relevance team in Elasticsearch v8.18.1 v8.19.0 v9.0.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] SearchTimeoutIT testSuggestTimeoutWithPartialResults failing
3 participants