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

Refactored ClusterStateUpdateTask protection against execution on a non master #7511

Conversation

Projects
None yet
4 participants
@bleskes
Copy link
Member

bleskes commented Aug 29, 2014

Previous implementation used a marker interface and had no explicit failure call back for the case update task was run on a non master (i.e., the master stepped down after it was submitted). That lead to a couple of instance of checks.

This approach moves ClusterStateUpdateTask from an interface to an abstract class, which allows adding a flag to indicate whether it should only run on master nodes (defaults to true). It also adds an explicit onNoLongerMaster call back to allow different error handling for that case. This also removed the need for the NoLongerMaster.

Note: this pr is agains the feature/improve_zen branch , as part of the review process of #7493

[Cluster] Refactored ClusterStateUpdateTask protection against execut…
…ion on a non master

Previous implementation used a marker interface and had no explicit failure call back for the case update task was run on a non master (i.e., the master stepped down after it was submitted). That lead to a couple of instance of checks.

This approach moves ClusterStateUpdateTask from an interface to an abstract class, which allows adding a flag to indicate whether it should only run on master nodes (defaults to true). It also adds an explicit onNoLongerMaster call back to allow different error handling for that case. This also removed the need for the  NoLongerMaster.
@martijnvg

This comment has been minimized.

Copy link
Member

martijnvg commented Aug 29, 2014

LGTM

/**
* indicates whether this task should only run if current node is master
*/
public boolean runOnlyOnMaster() {

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 1, 2014

Contributor

I think we can make this more crisp maybe masterOnly() or isMasterOnly()?

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 1, 2014

Author Member

Yeah - I debated this and decided to opt for clarity at the price of verbosity - this things are complex enough. isMasterOnly or masterOnly raises to me the question of what does it mean for an updateTask. I'm fine with changing it if other people feel that masterOnly is more intuitive.


/**
* Called when the result of the {@link #execute(ClusterState)} have been processed
* properly by all listeners.
*/
void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState);
abstract public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState);

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 1, 2014

Contributor

can we try to add visibility first?

This comment has been minimized.

Copy link
@bleskes

bleskes Sep 1, 2014

Author Member

sure.

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 1, 2014

left some minor comments but I like this a lot!

bleskes added a commit that referenced this pull request Sep 1, 2014

[Cluster] Refactored ClusterStateUpdateTask protection against execut…
…ion on a non master

Previous implementation used a marker interface and had no explicit failure call back for the case update task was run on a non master (i.e., the master stepped down after it was submitted). That lead to a couple of instance of checks.

This approach moves ClusterStateUpdateTask from an interface to an abstract class, which allows adding a flag to indicate whether it should only run on master nodes (defaults to true). It also adds an explicit onNoLongerMaster call back to allow different error handling for that case. This also removed the need for the  NoLongerMaster.

Closes #7511
@bleskes

This comment has been minimized.

Copy link
Member Author

bleskes commented Sep 1, 2014

Thx simon. pushed to 34f4ca7

@bleskes bleskes closed this Sep 1, 2014

@bleskes bleskes deleted the bleskes:non_master_update_task_rewrite branch Sep 1, 2014

bleskes added a commit to bleskes/elasticsearch that referenced this pull request Sep 1, 2014

[Cluster] Refactored ClusterStateUpdateTask protection against execut…
…ion on a non master

Previous implementation used a marker interface and had no explicit failure call back for the case update task was run on a non master (i.e., the master stepped down after it was submitted). That lead to a couple of instance of checks.

This approach moves ClusterStateUpdateTask from an interface to an abstract class, which allows adding a flag to indicate whether it should only run on master nodes (defaults to true). It also adds an explicit onNoLongerMaster call back to allow different error handling for that case. This also removed the need for the  NoLongerMaster.

Closes elastic#7511

@bleskes bleskes added v1.4.0 labels Sep 2, 2014

bleskes added a commit that referenced this pull request Sep 8, 2014

[Cluster] Refactored ClusterStateUpdateTask protection against execut…
…ion on a non master

Previous implementation used a marker interface and had no explicit failure call back for the case update task was run on a non master (i.e., the master stepped down after it was submitted). That lead to a couple of instance of checks.

This approach moves ClusterStateUpdateTask from an interface to an abstract class, which allows adding a flag to indicate whether it should only run on master nodes (defaults to true). It also adds an explicit onNoLongerMaster call back to allow different error handling for that case. This also removed the need for the  NoLongerMaster.

Closes #7511

@clintongormley clintongormley changed the title [Cluster] Refactored ClusterStateUpdateTask protection against execution on a non master Resiliency: Refactored ClusterStateUpdateTask protection against execution on a non master Sep 8, 2014

@clintongormley clintongormley changed the title Resiliency: Refactored ClusterStateUpdateTask protection against execution on a non master Refactored ClusterStateUpdateTask protection against execution on a non master Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.