Skip to content

Conversation

DaveCTurner
Copy link
Contributor

Similarly to #92987, recent improvements to the primitives for writing async code (particularly #92452 and #92620) mean that we can enormously simplify TransportBroadcastByNodeAction. In particular, we can avoid accumulating an intermediate array of responses for later processing in favour of just accumulating the successes and failures into their final separate lists. We also no longer need to use a separate NodesResponseTracker to discard responses on cancellation. Finally, we can now discard shard-level responses more promptly on cancellation.

Similarly to elastic#92987, recent improvements to the primitives for writing
async code (particularly elastic#92452 and elastic#92620) mean that we can enormously
simplify `TransportBroadcastByNodeAction`. In particular, we can avoid
accumulating an intermediate array of responses for later processing in
favour of just accumulating the successes and failures into their final
separate lists. We also no longer need to use a separate
`NodesResponseTracker` to discard responses on cancellation. Finally, we
can now discard shard-level responses more promptly on cancellation.
@DaveCTurner DaveCTurner added :Data Management/Stats Statistics tracking and retrieval APIs >refactoring v8.7.0 labels Feb 2, 2023
@DaveCTurner DaveCTurner requested a review from gmarouli February 2, 2023 21:03
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Feb 2, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

Copy link
Contributor

@gmarouli gmarouli left a comment

Choose a reason for hiding this comment

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

LGTM, great to see this code simplified and I really like the naming pattern executeAsCoordinatingNode and executeAsDataNode! 🚀

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 7, 2023
@DaveCTurner
Copy link
Contributor Author

@elasticmachine update branch

@elasticsearchmachine elasticsearchmachine merged commit f6eb279 into elastic:main Feb 7, 2023
@DaveCTurner DaveCTurner deleted the 2023-02-02-BroadcastByNodeAction-streamlining branch February 7, 2023 14:02
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request May 23, 2023
Each `TransportTasksAction` fans-out to multiple nodes, accumulates
responses and retains them until all the nodes have responded, and then
converts the responses into a final result.

Similarly to elastic#92987 and elastic#93484, we should accumulate the responses in a
structure that doesn't require so much copying later on, and should drop
the received responses if the task is cancelled while some nodes'
responses are still pending.
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request May 26, 2023
We have this somewhat-complex pattern in 3 places already, and elastic#96279
will introduce a couple more, so this commit extracts it as a dedicated
utility.

Relates elastic#92987
Relates elastic#93484
DaveCTurner added a commit that referenced this pull request May 26, 2023
We have this somewhat-complex pattern in 3 places already, and #96279
will introduce a couple more, so this commit extracts it as a dedicated
utility.

Relates #92987
Relates #93484
elasticsearchmachine pushed a commit that referenced this pull request May 30, 2023
Each `TransportTasksAction` fans-out to multiple nodes, accumulates
responses and retains them until all the nodes have responded, and then
converts the responses into a final result.

Similarly to #92987 and #93484, we should accumulate the responses in a
structure that doesn't require so much copying later on, and should drop
the received responses if the task is cancelled while some nodes'
responses are still pending.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Data Management/Stats Statistics tracking and retrieval APIs >refactoring Team:Data Management Meta label for data/management team v8.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants