Collect warnings in compute service #103031
Conversation
| /** | ||
| * A helper class that can be used to collect and merge response headers from multiple child requests. | ||
| */ | ||
| public final class ResponseHeadersCollector { |
There was a problem hiding this comment.
This class was extracted from DriverRunner.
|
Hi @dnhatn, I've created a changelog YAML for you. |
|
Hi @dnhatn, I've updated the changelog YAML for you. |
elasticsearchmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
| internalCluster().ensureAtLeastNumDataNodes(2); | ||
| final String node1, node2; | ||
| if (randomBoolean()) { | ||
| internalCluster().ensureAtLeastNumDataNodes(2); |
There was a problem hiding this comment.
++, I fixed this in 7948638
| client().admin().indices().prepareRefresh("index-1", "index-2").get(); | ||
|
|
||
| EsqlQueryRequest request = new EsqlQueryRequest(); | ||
| request.query("FROM index-* | EVAL ip = to_ip(host) | STATS s = COUNT(*) by ip | KEEP ip | LIMIT 100"); |
There was a problem hiding this comment.
Do we need the STATS here, given that the to_ip() will fail for every document? (just curious)
There was a problem hiding this comment.
We need to add a STATS for executing eval on data nodes. This is needed to reproduce the issue.
| .toList(); | ||
| int expectedWarnings = Math.min(20, numDocs1 + numDocs2); | ||
| // we cap the number of warnings per node | ||
| assertThat(warnings.size(), greaterThanOrEqualTo(expectedWarnings)); |
There was a problem hiding this comment.
Isn't warnings.size() == expectedWarnings + 1 (the + 1 being the header detailing the expression that failed)?
There was a problem hiding this comment.
We cap the number of warnings per execution pipeline, and we don't limit them again when merging the result on the coordinator; hence, we need to relax the assertion here.
There was a problem hiding this comment.
Ah, true (and it was right there in the comment). Thanks!
|
Backport PR: #103079 |
We have implemented elastic#99927 in DriverRunner. However, we also need to implement this in ComputeService, where we spawn multiple requests to avoid losing response headers. Relates elastic#99927 Closes elastic#100163 Closes elastic#102982 Closes elastic#102871 Closes elastic#103028
Re-enable a couple of warning header checking tests that have been disabled, but mistakenly without linking the issue (elastic#100163), which lead to them being kept disabled also after the fix (elastic#103031).
We have implemented #99927 in DriverRunner. However, we also need to implement this in ComputeService, where we spawn multiple requests to avoid losing response headers.
Relates #99927
Closes #100163
Closes #102982
Closes #102871
Closes #103028