Loading…
Accumulated improvements to ZenDiscovery #7493
7 participants
|
this
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
maybe call this I also think instead of all the instanceof checks we should maybe make this an abstract class and add a method to ClusterStateUpdateTask maybe? I like the fact that you indicate the criteria (master or not) in the class name. I also don't like the instanceof checks. Perhaps keep the current name and add a method call
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
can we maybe do sure. will do.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
I'd appreciate if we can do
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
are we sure the It can't. many things rely on that fact... ok fine! :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
can we maybe make this a constant? the setting I mean? unrelated but will do (will also convert it to a full path)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
can we use a switch / case statement here it's easier to read and 1.7 supports strings ++
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
can we make those all constants? and does it make sense to randomize some of them? I don't think we should randomize this one. We do want to rarely randomize the
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
should we validate this setting? ie can add.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
can we make the will do.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
should we do this in a try / catch fashion? and make sure we call it on all of them? ++
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
it might be good to have an assertion somewhere that make sure it's not there? will see where it makes sense to add this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
maybe we can turn this around and do if (electMaster.hasEnoughMasterNodes(possibleMasterNodes)) {
// lets tie break between discovered nodes
return electMaster.electMaster(possibleMasterNodes);
} else {
logger.trace("not enough master nodes [{}]", possibleMasterNodes);
return null;
}will do
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
maybe add a logging statement here? rather not because this ping happens every second. I think it will be rarely useful..
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
I start to see this a lot, can we have a static helper somewhere?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
it doens't matter if that one overflows no? I mean grows larger than Not really because it doesn't hurt to do the check twice. Rather be more resilient to errors exceptions etc.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
can't this be list from the beginning? I just followed the existing pattern. Will double check why it is so. I think it's OK? We want to prefer safety and copy the list anyway because we changing it's content. If that's the case might as well accept any Iterable. This code is rarely called so performance is secondary imho.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
can we make this a constant? and Ideally not using componentSettings it's so confusing +1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
I am not sure I understand this change here? This done to detect master problems faster. Before we waited 1 second (default ping interval) but we know here that the master is already active - we can start pinging immediately.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
maybe it makes sense to put this logic somehwere else to make sure we know it can be null? I can put it on the request deserialization but I think in this case it's better not to hide the fact that the request didn't have a clusterName with it? I'll add a comment as to how it can be null..
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
I can't see who uses this? The github diff is hard to read but it's used here: https://github.com/elasticsearch/elasticsearch/pull/7493/files#diff-b6157c9d93b838bb6020bbab99bfe7b6R399
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
it took me a while to figure out what all these different node lists / sets are maybe you can find a better name for agreed. Will change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
|
oh is this change BW compatible? The LocalTransport is only used for testing and only works within a single JVMs - so I think bwc is not an issue and we should prefer cleaner code?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
I did a first review round and left some mostly cosmetic comments. I don't know that code well enough to really review it deeply but given the fact that the development on the branch went through several review rounds is a good indicator. I think we are close here/
the changes here LGTM thanks!
typo s/removeDistruptionSchemeFromNode/removeDisruptionSchemeFromNode
clintongormley
changed the title from [Discovery] accumulated improvements to ZenDiscovery to Resiliency: Accumulated improvements to ZenDiscovery
clintongormley
changed the title from Resiliency: Accumulated improvements to ZenDiscovery to Accumulated improvements to ZenDiscovery
-
don't perform full recovery when minimum master nodes are not met, keep the state around and use it once elected as master
-
-
defaults to false since there is still work left to properly make it work
-
…t shows reads do execute (partially) when m_m_n isn't met
-
…iscoveryWithNetworkFailuresTests only.
-
…n from false to true in zen discovery. Added AwaitFix for the FullRollingRestartTests.
-
…revious known nodes.
-
…odes that are not in the latestDiscoNodes list. Only the previous master node has been removed, so only shards allocated to that node will get failed. This would have happened anyhow on later on when AllocationService#reroute is invoked (for example when a cluster setting changes or another cluster event), but by cleaning the routing table pro-actively, the stale routing table is fixed sooner and therefor the shards that are not accessible anyhow (because the node these shards were on has left the cluster) will get re-assigned sooner.
-
Introduced with: 11a3201
-
… longer master When a node steps down from being a master (because, for example, min_master_node is breached), it may still have cluster state update tasks queued up. Most (but not all) are tasks that should no longer be executed as the node no longer has authority to do so. Other cluster states updates, like electing the current node as master, should be executed even if the current node is no longer master. This commit make sure that, by default, `ClusterStateUpdateTask` is not executed if the node is no longer master. Tasks that should run on non masters are changed to implement a new interface called `ClusterStateNonMasterUpdateTask` Closes #6230
-
…e fact the master left
-
…ulated network split.
-
…is simulated
-
…ted node(s) rejoin the cluster after network segmentation and when the elected master node ended up on the lesser side of the network segmentation.
-
-
…ble at runtime.
-
… a first update from a new master We have an optimization which compares routing/meta data version of cluster states and tries to reuse the current object if the versions are equal. This can cause rare failures during recovery from a minimum_master_node breach when using the "new light rejoin" mechanism and simulated network disconnects. This happens where the current master updates it's state, doesn't manage to broadcast it to other nodes due to the disconnect and then steps down. The new master will start with a previous version and continue to update it. When the old master rejoins, the versions of it's state can equal but the content is different. Also improved DiscoveryWithNetworkFailuresTests to simulate this failure (and other improvements) Closes #6466
-
…g in data consistency test
-
… initializing shards before comparing cluster states
-
…disconnect default to false The previous default was true, which means that after a node disconnected event we try to connect to it as an extra validation. This can result in slow detection of network partitions if the extra reconnect times out before failure. Also added tests to verify the settings' behaviour
-
…use local node is no longer master
-
If the master FD flags master as gone while there are still pending cluster states, the processing of those cluster states we re-instate that node a master again. Closes #6526
-
This commit adds the notion of ServiceDisruptionScheme allowing for introducing disruptions in our test cluster. This abstraction as used in a couple of wrappers around the functionality offered by MockTransportService to simulate various network partions. There is also one implementation for causing a node to be slow in processing cluster state updates. This new mechnaism is integrated into existing tests DiscoveryWithNetworkFailuresTests. A new test called testAckedIndexing is added to verify retrieval of documents whose indexing was acked during various disruptions. Closes #6505
-
-
…dIndexing test: * waiting time should be long enough depending on the type of the disruption scheme * MockTransportService#addUnresponsiveRule if remaining delay is smaller than 0 don't double execute transport logic
-
Accumulate expected shard failures to log later
-
Also improved timeouts & logs
-
The test is currently unstable and needs some more work
-
-
…is updatable at runtime.
-
-
-
Moved testVerifyApiBlocksDuringPartition to test blocks rather then rely on specific API rejections. Did some cleaning while at it.
-
-
-
…fter partition
-
After a node joins the clusters, it starts pinging the master to verify it's health. Before, the cluster join request was processed async and we had to give some time to complete. With #6480 we changed this to wait for the join process to complete on the master. We can therefore start pinging immediately for fast detection of failures. Similar change can be made to the Node fault detection from the master side. Closes #6706
-
This is to allow the master election to complete on the chosen master. Relates to #6706
-
This is important to for proper primary allocation decisions
-
… active.
-
the current 30s addition is tricky because we use 30s as timeout in many places...
-
…StateNonMasterUpdateTask
-
Closes #6890
-
… local node is no longer master Since it runs in a background thread after a node is added, or submits a cluster state update when a node leaves, it may be that by the time it is executed the local node is no longer master.
-
…s circular references
-
added explicit cleaning of temp unicast ping results reduce gateway local.list_timeout to 10s. testVerifyApiBlocksDuringPartition: verify master node has stepped down before restoring partition
-
…sters to ping (ping responses did include master node) then these nodes should be resolved. After the findMaster() call we try to connect to the node and if it isn't the master we start looking for a new master via pinging again. Closes #6904
-
…sterUpdateTask
-
…er node.
-
… up electing ourselves without any form of verification.
-
Currently, pinging results are only used if the local node is elected master or if they detect another *already* active master. This has the effect that master election requires two pinging rounds - one for the elected master to take is role and another for the other nodes to detect it and join the cluster. We can be smarter and use the election of the first round on other nodes as well. Those nodes can try to join the elected master immediately. There is a catch though - the elected master node may still be processing the election and may reject the join request if not ready yet. To compensate a retry mechanism is introduced to try again (up to 3 times by default) if this happens. Closes #6943
-
Reduced expected time to heal to 0 (we interrupt and wait on stop disruption). It was also wrongly indicated in seconds. We didn't properly wait between slow cluster state tasks
-
After master election, nodes send join requests to the elected master. Master is then responsible for publishing a new cluster state which sets the master on the local node's cluster state. If something goes wrong with the cluster state publishing, this process will not successfully complete. We should check it after the join request returns and if it failed, retry pinging. Closes #6969
-
it probably comes wrapped in a remote exception, which we should unwrap in order to detect it..., also, simplified a bit the retry logic
-
In large clusters when a new elected master is chosen, there are many join requests to handle. By batching them up the the cluster state doesn't get published for each individual join request, but many handled at the same time, which results into a single new cluster state which ends up be published. Closes #6984
-
…ng / after a master re-election.
-
…ake cluster name into account. Both master fault detection and nodes fault detection request should also send the cluster name, so that on the receiving side the handling of these requests can be failed with an error. This error can be caught on the sending side and for master fault detection the node can fail the master locally and for nodes fault detection the node can be failed. Note this validation will most likely never fail in a production cluster, but in during automated tests where cluster / nodes are created and destroyed very frequently.
-
Also rename DiscoveryWithNetworkFailuresTests to DiscoveryWithServiceDisruptions which better suites what we do.
-
…detecting ping request The cluster state version allows resolving the case where a old master node become unresponsive and later wakes up and pings all the nodes in the cluster, allowing the newly elected master to decide whether it should step down or ask the old master to rejoin.
-
…a node.
-
-
…ering a node. Requests are handled by the worked thread pool of the target node instead of the generic thread pool of the source node. Also this change is required in order to make GC disruption work with local transport. Previously the handling of the a request was performed on on a node that that was being GC disrupted, resulting in some actions being performed while GC was being simulated.
-
…viceDisruptions#testMasterNodeGCs now local transport use worker threads.
-
-
…e is a no master block
-
…fter publishing timed out.
-
At the moment, when a node looses connection to the master (due to a partition or the master was stopped), we ping the unicast hosts in order to discover other nodes and elect a new master or get of another master than has been elected in the mean time. This can go wrong if all unicast targets are on the same side of a minority partition and therefore will never rejoin once the partition is healed. Closes #7336
-
-
-
-
…ected if pings indicate an active master
-
Used as null value for cluster state versions.
-
They share a lot of settings and some logic. Closes #7512
-
…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
This PR contains the accumulated work from the feautre/improve_zen branch. Here are the highlights of the changes:
Testing infra
Zen Discovery