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

Add two phased commit to Cluster State publishing #13062

Merged
merged 19 commits into from Sep 14, 2015

Conversation

Projects
None yet
5 participants
@bleskes
Copy link
Member

commented Aug 23, 2015

When publishing a new cluster state, the master will send it to all the node of the cluster, noting down how many master nodes responded successfully. The nodes do not yet process the new cluster state, but rather park it in memory. As soon as at least minimum master nodes have ack-ed the cluster state change, it is committed and a commit request is sent to all the node that responded so far (and will respond in the future). Once receiving the commit requests the nodes continue to process the cluster state change as they did before this change.

A few notable comments:

  1. For this change to have effect, min master nodes must be configured.
  2. All basic cluster state validation is done in the first phase of publish and is thus now part of ShardOperationResult
  3. A new COMMIT_TIMEOUT settings is introduced, dictating how long a master should wait for nodes to ack the first phase. Unlike PUBLISH_TIMEOUT, if waiting for a commit times out, the cluster state change will be rejected.
  4. Failing to achieve a min master node of acks, will cause the master to step down as it clearly doesn't have enough active followers.
  5. Previously there was a short window between the moment a master lost it's followers and it stepping down because of node fault detection failures. In this short window, the master could process any change (but fail to publish it). This PR closes this gap to 0.

I still have one no commit and some docs to add but I think we can start the review cycles.

@brwe @imotov and @jasontedor - can you have a careful look when you have time?

@bleskes bleskes changed the title Add two phased to Cluster State publishing Add two phased commit to Cluster State publishing Aug 23, 2015

@@ -57,6 +69,7 @@ public DiscoverySettings(Settings settings, NodeSettingsService nodeSettingsServ
nodeSettingsService.addListener(new ApplySettings());
this.noMasterBlock = parseNoMasterBlock(settings.get(NO_MASTER_BLOCK, DEFAULT_NO_MASTER_BLOCK));
this.publishTimeout = settings.getAsTime(PUBLISH_TIMEOUT, publishTimeout);
this.commitTimeout = settings.getAsTime(COMMIT_TIMEOUT, publishTimeout);

This comment has been minimized.

Copy link
@brwe

brwe Aug 24, 2015

Contributor

should this be settings.getAsTime(COMMIT_TIMEOUT, commitTimeout); ?

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 24, 2015

Author Member

ykes. Yes… :(

On 24 Aug 2015, at 11:57, Britta Weber notifications@github.com wrote:

In core/src/main/java/org/elasticsearch/discovery/DiscoverySettings.java:

@@ -57,6 +69,7 @@ public DiscoverySettings(Settings settings, NodeSettingsService nodeSettingsServ
nodeSettingsService.addListener(new ApplySettings());
this.noMasterBlock = parseNoMasterBlock(settings.get(NO_MASTER_BLOCK, DEFAULT_NO_MASTER_BLOCK));
this.publishTimeout = settings.getAsTime(PUBLISH_TIMEOUT, publishTimeout);

  •    this.commitTimeout = settings.getAsTime(COMMIT_TIMEOUT, publishTimeout);
    

should this be settings.getAsTime(COMMIT_TIMEOUT, commitTimeout); ?


Reply to this email directly or view it on GitHub.

/* The cluster name can still be null if the state comes from a node that is prev 1.1.1*/
if (incomingClusterName != null && !incomingClusterName.equals(this.clusterName)) {
logger.warn("received cluster state from [{}] which is also master but with a different cluster name [{}]", newClusterState.nodes().masterNode(), incomingClusterName);
newStateProcessed.onNewClusterStateFailed(new IllegalStateException("received state from a node that is not part of the cluster"));

This comment has been minimized.

Copy link
@brwe

brwe Aug 24, 2015

Contributor

why can we just remove this check?

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 25, 2015

Author Member

This one is moved to PublishClusterStateAction#validateIncomingState

@bleskes

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2015

@brwe @imotov @jasontedor FYI I removed the no commit.

String stateUUID;

public CommitClusterStateRequest() {
}

This comment has been minimized.

Copy link
@imotov

imotov Aug 24, 2015

Member

This constructor doesn't seem to be necessary.

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 26, 2015

Author Member

it's needed for the request to be created on the receiving node. It's used by reflection.

}
}
timedout = committedOrFailedLatch.await(commitTimeout.millis(), TimeUnit.MILLISECONDS) == false;
} catch (InterruptedException e) {

This comment has been minimized.

Copy link
@imotov

imotov Aug 24, 2015

Member

Maybe we should restore interrupted state or at least explain why we ignore it here?

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 26, 2015

Author Member

Added a note

logger.debug("--> publishing version [{}], UUID [{}]", state.version(), state.stateUUID());
boolean success;
try {
publishState(master.action, state, master.clusterState, 2).await(1, TimeUnit.HOURS);

This comment has been minimized.

Copy link
@imotov

imotov Aug 24, 2015

Member

This test fails here if I run

mvn test -Pdev -pl org.elasticsearch:elasticsearch -Dtests.seed=8302BF8B33D58713 -Dtests.class=org.elasticsearch.discovery.zen.publish.PublishClusterStateActionTests -Des.logger.level=ERROR -Dtests.assertion.disabled=false -Dtests.security.manager=true -Dtests.heap.size=512m -Dtests.locale=ar_QA -Dtests.timezone=Pacific/Kwajalein -Dtests.iters=200

a few times. Here is a typical error: https://gist.github.com/imotov/a1512b9a27a2854c90f5

@bleskes

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2015

I update the docs. @clintongormley @jasontedor - I would love a native English speaker to review it - can you take a look?

@jasontedor

This comment has been minimized.

Copy link
Member

commented Aug 25, 2015

@bleskes I left a review of the updated docs but I'm still in progress on a review of the code.

to 30 seconds and can be changed dynamically through the
<<cluster-update-settings,cluster update settings api>>
the other nodes in the cluster. Each node receives the publish message, acknowledges
it but do *not* yet apply it. If the master does not receive acknowledgement from

This comment has been minimized.

Copy link
@clintongormley

clintongormley Aug 25, 2015

Member

do -> does

change is rejected.

Once enough nodes have responded, the cluster state is committed and a message will
be sent to all the nodes. The nodes then proceed and apply the new cluster state to their

This comment has been minimized.

Copy link
@clintongormley

clintongormley Aug 25, 2015

Member

proceed and -> proceed to

=== Use two phase commit for Cluster State publishing (STATUS: ONGOING)

A master node in Elasticsearch continuously https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-discovery-zen.html#fault-detection[monitors the cluster nodes]
and removes any node from the cluster that doesn't respond to it's pings in a timely

This comment has been minimized.

Copy link
@clintongormley

clintongormley Aug 25, 2015

Member

it's -> its


A master node in Elasticsearch continuously https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-discovery-zen.html#fault-detection[monitors the cluster nodes]
and removes any node from the cluster that doesn't respond to it's pings in a timely
fashion. If the master is left with less nodes than the `discovery.zen.minimum_master_nodes`

This comment has been minimized.

Copy link
@clintongormley

clintongormley Aug 25, 2015

Member

less -> fewer

fashion. If the master is left with less nodes than the `discovery.zen.minimum_master_nodes`
settings, it will step down and a new master election will start.

When a network partition occurs causing a master to loose many followers, there is a

This comment has been minimized.

Copy link
@clintongormley

clintongormley Aug 25, 2015

Member

When a network partition causes a master node to lose many followers, there is a short window in time until the node loss is detected and the master steps down.

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 25, 2015

Author Member

better. thx.

@clintongormley

This comment has been minimized.

Copy link
Member

commented Aug 25, 2015

left some minor docs suggestions

logger.debug("publishing cluster state version [{}]", newClusterState.version());
try {
discoveryService.publish(clusterChangedEvent, ackListener);
} catch (Throwable t) {

This comment has been minimized.

Copy link
@brwe

brwe Aug 26, 2015

Contributor

I guess we need that to catch the FailedToCommitException? If so, why not only catch that?

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 26, 2015

Author Member

That's a good one. I've tightened things up to make sure can rely on FaileToCommitException to indicate something wrong happened before committing and we can safely reject the CS (it will not be committed on any node)

return committed;
}

synchronized public void onNodeSendAck(DiscoveryNode node) {

This comment has been minimized.

Copy link
@brwe

brwe Aug 26, 2015

Contributor

can we give that a better name that describes what the function does? something like checkCommittedAndSendCommitIfSo().

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 26, 2015

Author Member

The naming suggest when the methods are called (when a node acked the sent CS). It's what we use in many other places, I think it's OK?

This comment has been minimized.

Copy link
@brwe

brwe Aug 27, 2015

Contributor

I understand we sometimes have to give vague names to methods when they are needed for some sort of abstraction but this is not the case here. We have the following methods:

  • onNodeSendAck: sends a commit to the node in case state is committed and otherwise check if state is committed
  • onMasterNodeSendAck the actual check if the state is committed now
  • onMasterNodeDone countdown outstanding send requests and additional check if too many master nodes failed and quorum not met
  • onNodeSendFailed countdown outstanding publish requests and calls onMasterNodeDone for the extra check

None of them are ever overwritten or in any other way part of an abstraction that would require them to have generic names. I find it hard to read the code as is and think it would be much easier if there was at least a hint in the method names.

}
}

synchronized private void onMasterNodeSendAck(DiscoveryNode node) {

This comment has been minimized.

Copy link
@brwe

brwe Aug 26, 2015

Contributor

same as above, function name says nothing about what it does.

void validateIncomingState(ClusterState incomingState, ClusterState lastSeenClusterState) {
final ClusterName incomingClusterName = incomingState.getClusterName();
if (!incomingClusterName.equals(PublishClusterStateAction.this.clusterName)) {
logger.warn("received cluster state from [{}] which is also master but with a different cluster name [{}]", incomingState.nodes().masterNode(), incomingClusterName);

This comment has been minimized.

Copy link
@brwe

brwe Aug 26, 2015

Contributor

why the PublishClusterStateAction.this. ?

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 26, 2015

Author Member

left over copy paste. I'll remove

synchronized private void onMasterNodeDone(DiscoveryNode node) {
pendingMasterNodes--;
if (pendingMasterNodes == 0 && neededMastersToCommit > 0) {
markAsFailed("All master nodes acked or failed but [" + neededMastersToCommit + "] acks are still needed");

This comment has been minimized.

Copy link
@brwe

brwe Aug 26, 2015

Contributor

what does this message mean? did the master nodes ack or not?

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 26, 2015

Author Member

some did, some failed. I'll change it and see if it's better.

@bleskes

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2015

@imotov @brwe pushed another commit addressing comments so far

// ignore & restore interrupt
Thread.currentThread().interrupt();
} catch (IOException e) {
throw new ElasticsearchException("failed to serialize cluster_state for publishing to node {}", e, node);
}
}
}

private void sendFullClusterState(ClusterState clusterState, @Nullable Map<Version, BytesReference> serializedStates,

This comment has been minimized.

Copy link
@imotov

imotov Aug 26, 2015

Member

It looks like with the latest changes serializedStates can no longer be null, so we should probably remove Nullable here.

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 27, 2015

Author Member

correct. Removed.

bytes = serializedStates.get(node.version());
}
DiscoveryNode node, TimeValue publishTimeout, SendingController sendingController) {
BytesReference bytes = serializedStates.get(node.version());
if (bytes == null) {
try {
bytes = serializeFullClusterState(clusterState, node.version());

This comment has been minimized.

Copy link
@imotov

imotov Aug 26, 2015

Member

if (serializedStates != null) { is no longer needed

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 27, 2015

Author Member

confused about this one?

This comment has been minimized.

Copy link
@imotov

imotov Aug 27, 2015

Member

Sorry, it's not visible in github, but on the next line after this one there is a check if serializedStates is null or not. However, it cannot be null here. If it was null it would have failed with NPE 4 lines above where we try to retrieve version from it.

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 27, 2015

Author Member

I see. removed.

// and not log an error if it arrives after the timeout
transportService.sendRequest(node, COMMIT_ACTION_NAME,
new CommitClusterStateRequest(clusterState.stateUUID()),
options, // no need to compress, we already compressed the bytes

This comment has been minimized.

Copy link
@imotov

imotov Aug 26, 2015

Member

This comment doesn't make much sense here.

This comment has been minimized.

Copy link
@bleskes

bleskes Aug 27, 2015

Author Member

yeah, removed

import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.common.transport.DummyTransportAddress;
import org.elasticsearch.common.transport.TransportAddress;

This comment has been minimized.

Copy link
@imotov

imotov Aug 26, 2015

Member

Unused import

@@ -41,9 +45,10 @@ public void testShouldIgnoreNewClusterState() {
ClusterName clusterName = new ClusterName("abc");

DiscoveryNodes.Builder currentNodes = DiscoveryNodes.builder();
currentNodes.masterNodeId("a");
currentNodes.masterNodeId("a").put(new DiscoveryNode("a", DummyTransportAddress.INSTANCE, Version.CURRENT));
;

This comment has been minimized.

Copy link
@imotov

imotov Aug 26, 2015

Member

Unused ";"

@brwe

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2015

Just #13062 (comment) left and I am not too passionate about it. LGTM too otherwise.

bleskes added some commits Aug 19, 2015

@bleskes bleskes force-pushed the bleskes:discovery_two_phase_pub branch to 218979d Aug 28, 2015

@bleskes

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2015

@brwe thx. I update the PR based on your last comment (and rebased to latest master)

bleskes added a commit to bleskes/elasticsearch that referenced this pull request Sep 11, 2015

Discovery: Add a dedicate queue for incoming ClusterStates
The initial implementation of two phase commit based cluster state publishing (elastic#13062) relied on a single in memory "pending" cluster state that is only processed by ZenDiscovery once committed by the master. While this is fine on it's own, it resulted in an issue with acknowledged APIs, such as the open index API, in the extreme case where a node falls behind and receives a commit message after a new cluster state has been published. Specifically:

1) Master receives and acked-API call and publishes cluster state CS1
2) Master waits for a min-master nodes to receives CS1 and commits it.
3) All nodes that have responded to CS1 are sent a commit message, however, node N didn't respond yet
4) Master waits for publish timeout (defaults to 30s) for all nodes to process the commit. Node N fails to do so.
5) Master publishes a cluster state CS2. Node N responds to cluster state CS1's publishing but receives cluster state CS2 before the commit for CS1 arrives.
6) The commit message for cluster CS1 is processed on node N, but fails because CS2 is pending. This caused the acked API in step 1 to return (but CS2 , is not yet processed).

In this case, the action indicated by CS1 is not yet executed on node N and therefore the acked API calls return pre-maturely. Note that once CS2 is processed but the change in CS1 takes effect (cluster state operations are safe to batch and we do so all the time).

An example failure can be found on: http://build-us-00.elastic.co/job/es_feature_two_phase_pub/314/

This commit extracts the already existing pending cluster state queue (processNewClusterStates) from ZenDiscovery into it's own class, which serves as a temporary container for in-flight cluster states. Once committed the cluster states are transferred to ZenDiscovery as they used to before. This allows "lagging" cluster states to still be successfully committed and processed (and likely to be ignored as a newer cluster state has already been processed).

As a side effect, all batching logic is now extracted from ZenDiscovery and is unit tested.

@bleskes bleskes merged commit 218979d into elastic:master Sep 14, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

bleskes added a commit that referenced this pull request Sep 14, 2015

Discovery: Add two phased commit to Cluster State publishing
When publishing a new cluster state, the master will send it to all the node of the cluster, noting down how many *master* nodes responded successfully. The nodes do not yet process the new cluster state, but rather park it in memory. As soon as at least minimum master nodes have ack-ed the cluster state change, it is committed and a commit request is sent to all the node that responded so far (and will respond in the future). Once receiving the commit requests the nodes continue to process the cluster state change as they did before this change.

A few notable comments:
1. For this change to have effect, min master nodes must be configured.
2. All basic cluster state validation is done in the first phase of publish and is thus now part of `ShardOperationResult`
3. A new `COMMIT_TIMEOUT` settings is introduced, dictating how long a master should wait for nodes to ack the first phase. Unlike `PUBLISH_TIMEOUT`, if waiting for a commit times out, the cluster state change will be rejected.
4. Failing to achieve a min master node of acks, will cause the master to step down as it clearly doesn't have enough active followers.
5. Previously there was a short window between the moment a master lost it's followers and it stepping down because of node fault detection failures. In this short window, the master could process any change (but fail to publish it). This PR closes this gap to 0.
6. A dedicated pending cluster states queue was added to keep pending non-comitted cluster states and manage the logic around processing committed cluster states. See #13303 for details.

Closes #13062 , Closes #13303
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.