ESQL: Fix async query inconsistent headers #135078
Conversation
ivancea
added
>bug
Team:Analytics
|
Hi @ivancea, I've created a changelog YAML for you. |
| public boolean isAsync() { | ||
| return isRunning; | ||
| return isAsync; |
There was a problem hiding this comment.
This was only being used on a test, so changing it is safe
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
ivancea
changed the title
| * </p> | ||
| */ | ||
| static <R> ActionListener<R> unwrapListener(ActionListener<R> listener) { | ||
| ActionListener<EsqlQueryResponse> unwrapListener(String asyncExecutionId, ActionListener<EsqlQueryResponse> listener) { |
There was a problem hiding this comment.
I could rename this method, listening to better names here. I was unironically considering switching it to "wrapListener()"
| */ | ||
| ASYNC_QUERY_STATUS_HEADERS, | ||
|
|
||
| /** |
There was a problem hiding this comment.
Do we need the ASYNC_QUERY_STATUS_HEADERS capability anymore?
There was a problem hiding this comment.
Unless in snapshot or some other special cases, we usually don't remove them.
Also, a 8.19 server could be executing tests against this one (rest compatibility tests), and this one should say "Yes, I can handle that".
| if (response.isAsync()) { | ||
| if (response.asyncExecutionId().isPresent()) { | ||
| String asyncExecutionId = response.asyncExecutionId().get(); | ||
| threadPool.getThreadContext().addResponseHeader(AsyncExecutionId.ASYNC_EXECUTION_ID_HEADER, asyncExecutionId); |
There was a problem hiding this comment.
Dummy question: why do we need this duplicated (in TransportEsqlAsyncGetResultsAction.java also)?
There was a problem hiding this comment.
Async queries have 2 endpoints: POST _query/async to execute the query, and GET _query/async/<id>.
The bug was that we already had the headers for query execution (Here I just refactored it a bit), but the "get" class wasn't setting them.
It worked when the GET returned a result, as the headers are stored with it. But before it had the result, there were no headers.
💔 Backport failed
You can use sqren/backport to manually backport by running |
Fixes elastic#135042 This PR: - Fixes the get-result not always returning the expected headers - Fixes the non-async query incorrectly returning the "is running" async header
Fixes elastic#135042 This PR: - Fixes the get-result not always returning the expected headers - Fixes the non-async query incorrectly returning the "is running" async header
Fixes elastic#135042 This PR: - Fixes the get-result not always returning the expected headers - Fixes the non-async query incorrectly returning the "is running" async header
* ESQL: Fix async query inconsistent headers (#135078) Fixes #135042 This PR: - Fixes the get-result not always returning the expected headers - Fixes the non-async query incorrectly returning the "is running" async header * Revert rest tests file * Added main changes on test class * Backported test for the headers fix * Remove unrelated changes
Fixes #135042
This PR: