Skip to content

Conversation

@dnhatn
Copy link
Member

@dnhatn dnhatn commented Feb 14, 2025

We should notify the listener provided in delegateResponse/delegateFailure, not the original listener.

@dnhatn dnhatn requested a review from smalyshev February 14, 2025 17:26
@dnhatn dnhatn marked this pull request as ready for review February 14, 2025 17:26
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 14, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@dnhatn dnhatn added the auto-backport Automatically create backport pull requests when merged label Feb 14, 2025
return ActionListener.runAfter(delegate.delegateResponse((inner, e) -> {
taskManager.cancelTaskAndDescendants(groupTask, reason, true, ActionListener.running(() -> {
EsqlCCSUtils.skipUnavailableListener(delegate, executionInfo, clusterAlias, EsqlExecutionInfo.Cluster.Status.PARTIAL)
EsqlCCSUtils.skipUnavailableListener(inner, executionInfo, clusterAlias, EsqlExecutionInfo.Cluster.Status.PARTIAL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the code, it seems like inner and delegate in this case are the same thing, am I wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it should, but it's more idiomatic to use the inner listener.

@dnhatn
Copy link
Member Author

dnhatn commented Feb 14, 2025

@idegtiarenko @smalyshev Thanks for reviews.

@dnhatn dnhatn merged commit b619e14 into elastic:main Feb 14, 2025
17 checks passed
@dnhatn dnhatn deleted the fix-listener branch February 14, 2025 20:41
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Feb 14, 2025
We should notify the listener provided in delegateResponse/delegateFailure, 
not the original listener.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

elasticsearchmachine pushed a commit that referenced this pull request Feb 14, 2025
We should notify the listener provided in delegateResponse/delegateFailure, 
not the original listener.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.19.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants