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

Response headers missing from search response #33936

Open
jimczi opened this issue Sep 21, 2018 · 2 comments
Open

Response headers missing from search response #33936

jimczi opened this issue Sep 21, 2018 · 2 comments
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities high hanging fruit Team:Core/Infra Meta label for core/infra team

Comments

@jimczi
Copy link
Contributor

jimczi commented Sep 21, 2018

Today we have the ability to preserve response headers if they are present on a transport protocol response. For instance search action relies on this feature to preserve depreciation warnings when search is broadcasted to all shards. However we had some failures where deprecation warnings are lost during a search:
#31116
#28600

In these tests some internal shard requests create a deprecation warning and other don't. Depending on the timing of the internal shard actions the expected deprecation warning is sometimes missing from the response. We start digging with @ywelsch and we think that only the headers of the last response received by the coordinating node are preserved. Usually all shards emit the same deprecation warning so this was uncaught until now but these tests seem to reveal something new.
@jimczi jimczi added >bug :Core/Infra/REST API REST infrastructure and utilities labels Sep 21, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@jimczi jimczi mentioned this issue Sep 21, 2018
Closed
This was referenced Dec 11, 2019
Closed
Merged
jtibshirani added a commit that referenced this issue Dec 11, 2019
@jtibshirani
In sparse vector REST tests, specify the index name in searches. (#50061
277880b 
)

The `sparse_vector` REST tests occasionally fail on 7.x because we don't receive the expected response headers with deprecation warnings.

One theory as to what is happening is that there is an extra empty index present in addition to the test index. Since the search doesn't specify an index name, it hits both the test index and this extra empty index and shard responses from the extra index don't produce deprecation warnings. If not all shard responses contain the warning headers, then certain deprecation warnings can be lost (due to the bug described in #33936).

This PR tries to harden the `sparse_vector` tests by always specifying the index name during a search. This doesn't fix the root causes of the issue, but is good practice and can help avoid intermittent failures.

Addresses #49383.
This was referenced Jan 29, 2020
Closed
Merged
jtibshirani added a commit that referenced this issue Jan 29, 2020
@jtibshirani
Always use one shard in vector REST tests. (#51643)
9dcc3ef 
This PR tries to address the intermittent vector test failures on 7.x by making
sure we create indices with one shard.

The fix is based on this theory as to what's happening:
* On 7.x, the default number of shards is 1, but in REST tests we randomly use
2 in order to cover the multiple shards case. In the failing test run, we use 2
shards and all documents end up on only one shard.
* During a search, the response from the empty shard doesn't produce
deprecation warnings because  we never try to execute the script. If not all
shard responses contain the warning headers, then certain deprecation warnings
can be lost (due to the bug described in #33936).

Addresses #50716.
Relates to #50061.
jtibshirani added a commit that referenced this issue Jan 29, 2020
@jtibshirani
Always use one shard in vector REST tests. (#51643)
730153c 
This PR tries to address the intermittent vector test failures on 7.x by making
sure we create indices with one shard.

The fix is based on this theory as to what's happening:
* On 7.x, the default number of shards is 1, but in REST tests we randomly use
2 in order to cover the multiple shards case. In the failing test run, we use 2
shards and all documents end up on only one shard.
* During a search, the response from the empty shard doesn't produce
deprecation warnings because  we never try to execute the script. If not all
shard responses contain the warning headers, then certain deprecation warnings
can be lost (due to the bug described in #33936).

Addresses #50716.
Relates to #50061.
jtibshirani added a commit that referenced this issue Jan 29, 2020
@jtibshirani
Always use one shard in vector REST tests. (#51643)
c9679c0 
This PR tries to address the intermittent vector test failures on 7.x by making
sure we create indices with one shard.

The fix is based on this theory as to what's happening:
* On 7.x, the default number of shards is 1, but in REST tests we randomly use
2 in order to cover the multiple shards case. In the failing test run, we use 2
shards and all documents end up on only one shard.
* During a search, the response from the empty shard doesn't produce
deprecation warnings because  we never try to execute the script. If not all
shard responses contain the warning headers, then certain deprecation warnings
can be lost (due to the bug described in #33936).

Addresses #50716.
Relates to #50061.
jimczi added a commit to jimczi/elasticsearch that referenced this issue Mar 27, 2020
@jimczi
Preserve final response headers in asynchronous search
e9f700c 
This change adds the response headers of the original search request
in the stored response in order to be able to restore them when retrieving a result
from the async-search index. It also ensures that response headers are preserved for
users that retrieve a final response on a running search task.
Partial response can eventually return response headers too but this change only ensures
that they are present when the response if final.

Relates elastic#33936
@jimczi jimczi mentioned this issue Mar 27, 2020
Merged
jimczi added a commit that referenced this issue Apr 7, 2020
@jimczi
Preserve final response headers in asynchronous search (#54349)
f3f1b69 
This change adds the response headers of the original search request
in the stored response in order to be able to restore them when retrieving a result
from the async-search index. It also ensures that response headers are preserved for
users that retrieve a final response on a running search task.
Partial response can eventually return response headers too but this change only ensures
that they are present when the response if final.

Relates #33936
jimczi added a commit that referenced this issue Apr 7, 2020
@jimczi
Preserve final response headers in asynchronous search (#54349)
c7ff67d 
This change adds the response headers of the original search request
in the stored response in order to be able to restore them when retrieving a result
from the async-search index. It also ensures that response headers are preserved for
users that retrieve a final response on a running search task.
Partial response can eventually return response headers too but this change only ensures
that they are present when the response if final.

Relates #33936
jimczi added a commit that referenced this issue Apr 7, 2020
@jimczi
Preserve final response headers in asynchronous search (#54349)
f9fb373 
This change adds the response headers of the original search request
in the stored response in order to be able to restore them when retrieving a result
from the async-search index. It also ensures that response headers are preserved for
users that retrieve a final response on a running search task.
Partial response can eventually return response headers too but this change only ensures
that they are present when the response if final.

Relates #33936
@rjernst rjernst added the Team:Core/Infra Meta label for core/infra team label May 4, 2020
@rjernst rjernst added the needs:triage Requires assignment of a team area label label Dec 3, 2020
@jaymode
Copy link
Member

jaymode commented Dec 10, 2020

I think this is a long standing limitation of how we deal with response headers during broadcast and scatter/gather listeners. A listener could be implemented that could handle the merging of response headers concurrently. Then we would need to adapt existing code to use this listener.

@jaymode jaymode added high hanging fruit and removed needs:triage Requires assignment of a team area label labels Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities high hanging fruit Team:Core/Infra Meta label for core/infra team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@rjernst @jaymode @elasticmachine @jimczi