Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove LegacyCTRAListener from ShardStateAction #83842

Merged
merged 3 commits into from
Feb 11, 2022
Merged

Remove LegacyCTRAListener from ShardStateAction #83842

merged 3 commits into from
Feb 11, 2022

Conversation

DaveCTurner
Copy link
Contributor

@DaveCTurner DaveCTurner commented Feb 11, 2022 •
edited
Loading

Relates #83784

@DaveCTurner DaveCTurner added :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) >refactoring v8.2.0 labels Feb 11, 2022
@DaveCTurner DaveCTurner requested a review from joegallo February 11, 2022 16:01
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Feb 11, 2022
@DaveCTurner DaveCTurner mentioned this pull request Feb 11, 2022
Closed
16 tasks
joegallo
joegallo approved these changes Feb 11, 2022
View reviewed changes
Copy link
Contributor

@joegallo joegallo left a comment

Choose a reason for hiding this comment

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

LGTM, but I find the .success(task, task) a bit bleh -- if this ends up being a popular pattern for handling these, then a .success(task) arity that juggles the types just so might be handy.

I like this pattern, though! It makes me want to revisit my two previous PRs and give them the same treatment.

@DaveCTurner
Copy link
Contributor Author

Changed my mind, I think this pattern is tricksy for the general reason that re-using listeners is tricksy. It'd be worth it if profiling shows we create too many objects here but otherwise I would like to keep things clear - see 5f93f8e.

@DaveCTurner
Copy link
Contributor Author

@elasticmachine please run elasticsearch-ci/bwc

@joegallo
Copy link
Contributor

Still LGTM.

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

Hit #83567 twice :(

@elasticmachine please run elasticsearch-ci/bwc

@elasticsearchmachine elasticsearchmachine merged commit f45621f into elastic:master Feb 11, 2022
@DaveCTurner DaveCTurner deleted the 2022-02-11-ShardStateAction-LegacyCTRAListener branch February 11, 2022 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joegallo joegallo joegallo approved these changes

Assignees
No one assigned
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) >refactoring Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DaveCTurner @joegallo @elasticmachine @elasticsearchmachine