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.

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

StartedShardUpdateTask task listener #82854

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

idegtiarenko
Copy link
Contributor

Today node removal tasks executed by the master have a separate
ClusterStateTaskListener to feed back the result to the requestor.
It'd be preferable to use the task itself as the listener.

Rel: #82644

Today node removal tasks executed by the master have a separate
ClusterStateTaskListener to feed back the result to the requestor.
It'd be preferable to use the task itself as the listener.
@idegtiarenko idegtiarenko added >refactoring :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.1.0 labels Jan 20, 2022
@@ -872,6 +850,43 @@ public int hashCode() {
}
}

public static class StartedShardUpdateTask implements ClusterStateTaskListener {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is similar to #82795,
though I wonder if it make sense to gentrify this class for both Started and Failed entry.

This will result in less code, however some of the declarations might get longer

Copy link
Contributor

Choose a reason for hiding this comment

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

s/gentrify/generify/?

I'm not sure there's much to be gained from extracting a common base class for the two requests at the moment. It wouldn't reduce any code duplication AFAIK. I think it makes more sense to aim for splitting ShardStateAction into separate action classes for the shard-started and shard-failed functionality since they're conceptually quite independent. The sendShardAction method is not really doing anything that TransportMasterNodeAction couldn't do today, although I would like to change the behaviour here so that when a new master is elected we send all the retries in a single transport request.

@idegtiarenko idegtiarenko marked this pull request as ready for review January 24, 2022 09:23
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

LGTM

@idegtiarenko idegtiarenko merged commit 6959203 into elastic:master Jan 24, 2022
@idegtiarenko idegtiarenko deleted the 82644_started_shard_entry branch January 24, 2022 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >refactoring Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants