Simplify ES|QL execution info #120418
Simplify ES|QL execution info #120418
Conversation
dnhatn
force-pushed
the
compute-listener
branch
from
134de75 to
0c24e2a
Compare
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
I've started reviewing, but can we hold off merging until Stas' PR merges, as we want to get those features in 8.18 - then this refactoring will need to encompass those changes as well. |
|
@quux00 I think either PR can be merged first. If this one gets merged first, I can help update Stas' PR, which should be quick. I'll need this change for the retry implementation. Thanks! |
|
@quux00 @smalyshev We need this PR to fix an issue in Stas's PR, so it needs to be merged first. Once it's in, I'll update Stas's PR. |
quux00
left a comment
quux00
left a comment
There was a problem hiding this comment.
Very clean and elegant refactoring. I left one comment so far. I plan to go over it again and make sure I get the flow.
| profiles, | ||
| took, | ||
| dataNodeResult.totalShards(), | ||
| dataNodeResult.totalShards() - dataNodeResult.skippedShards(), |
There was a problem hiding this comment.
In _search, I believe skipped shards are also considered successful. Do we want to preserve that behavior in ES|QL or have this new model?
| * of ESQL that does not record and return CCS metadata. Ensure that the local cluster {@link EsqlExecutionInfo} | ||
| * is properly updated with took time and shard info is left unset. | ||
| */ | ||
| public void testAcquireComputeCCSListenerWithComputeResponseFromOlderCluster() { |
There was a problem hiding this comment.
Do we need some variant of this test even with the new refactored code?
There was a problem hiding this comment.
I think we don't need these tests in the ComputerListener. I'll try adding unit tests for the ClusterComputeHandler instead. Can I defer this to a follow-up to unblock Stas's PR?
There was a problem hiding this comment.
OK, sounds good. I approved the PR.
dnhatn
added
the
auto-backport
|
@quux00 Thanks for reviewing. |
💚 Backport successful
|
Currently, ES|QL ExecutionInfo is updated in multiple places: on remote clusters, during the execution of data-node requests, in compute listeners, and on the coordinator node. This change consolidates updates to happen only on the coordinator, when receiving compute responses. This change should also make the compute listener easier to understand
Currently, ES|QL ExecutionInfo is updated in multiple places: on remote clusters, during the execution of data-node requests, in compute listeners, and on the coordinator node. This change consolidates updates to happen only on the coordinator, when receiving compute responses. This change should also make the compute listener easier to understand
3 participants
Currently, ES|QL ExecutionInfo is updated in multiple places: on remote clusters, during the execution of data-node requests, in compute listeners, and on the coordinator node. This change consolidates updates to happen only on the coordinator, when receiving compute responses. This change should also make the compute listener easier to understand.