Improve cluster update settings api #6244
Conversation
martijnvg
commented
May 20, 2014
martijnvg
commented
- If the minimum master nodes has been breached the reroute shouldn't be executed
- Cluster update settings call should also return in case of failures during the reroute.
| @@ -116,6 +116,11 @@ public void onAckTimeout() { | |||
| } | |||
|
|
|||
| private void reroute(final boolean updateSettingsAcked) { | |||
| if (!clusterService.state().nodes().localNodeMaster()) { | |||
There was a problem hiding this comment.
I think we should add a comment about min master nodes causing current node not to be master...
There was a problem hiding this comment.
Updated the PR with a commit
|
LGTM. Left one comment. I also think we can improve a bit the description of the PR (cluster update settings may not return on reroute failure etc.) |
| @@ -150,6 +155,7 @@ public TimeValue timeout() { | |||
| public void onFailure(String source, Throwable t) { | |||
| //if the reroute fails we only log | |||
| logger.debug("failed to perform [{}]", t, source); | |||
| listener.onFailure(t); | |||
There was a problem hiding this comment.
good catch! not 100% sure we should fail the whole request though, as the actual cluster update settings went well, although the following cluster reroute failed. The settings have effectively been applied...maybe we should return them doing something like this:
listener.onResponse(new ClusterUpdateSettingsResponse(transientUpdates.build(), persistentUpdates.build()));
But then we would not return any error and only log it...
There was a problem hiding this comment.
I think we should inform the caller that there was an error, if it fails here something doesn't add up. We can wrap it in another description that has a message indicating that only the reroute after update cluster settings update failed.
There was a problem hiding this comment.
I agree we should return the error as something wrong happened. But this way we miss returning the settings that have been changed. The cluster update settings call had an effect although we return an error, which is misleading. The problem is we cant return both an error and the settings that have changed...
|
Left one comment as well, agreed on improving the description of the PR. BTW I think the call would return in case of failure, but only after the ack timeout (with |
…ecuted and if that isn't the case skip reroute Invoke listener when reroute fails.
…ecuted and if that isn't the case skip reroute Invoke listener when reroute fails. Closes #6244
clintongormley
changed the title
clintongormley
changed the title
clintongormley
added
:Distributed/Distributed
