[Zen2] Introduce gossip-like discovery of master nodes by DaveCTurner · Pull Request #32246 · elastic/elasticsearch

DaveCTurner · 2018-07-20T15:33:13Z

This commit introduces the PeerFinder which can be used to collect the
identities of the master-eligible nodes in a masterless cluster, based on the
UnicastHostsProvider, the nodes in the ClusterState, and nodes that other
nodes have discovered.

This commit introduces the `PeerFinder` which can be used to collect the identities of the master-eligible nodes in a masterless cluster, based on the `UnicastHostsProvider`, the nodes in the `ClusterState`, and nodes that other nodes have discovered.

elasticmachine · 2018-07-20T15:33:15Z

Pinging @elastic/es-distributed

ywelsch

I've done an initial round. I've left mostly smaller comments, the general flow and the testing looks good.

ywelsch · 2018-07-30T07:05:29Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeerFinder.java

+
+public abstract class PeerFinder extends AbstractLifecycleComponent {
+
+    public static final String REQUEST_PEERS_ACTION_NAME = "internal:discovery/zen2/requestpeers";


should we remove the zen2 part here and just have this under discovery? Also maybe request_peers?

ywelsch · 2018-07-30T07:07:42Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeerFinder.java

+
+    // the time between attempts to find all peers
+    public static final Setting<TimeValue> CONSENSUS_FIND_PEERS_DELAY_SETTING =
+        Setting.timeSetting("discovery.zen2.find_peers_delay",


remove zen2?

ywelsch · 2018-07-30T07:08:14Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeerFinder.java

+    public static final String REQUEST_PEERS_ACTION_NAME = "internal:discovery/zen2/requestpeers";
+
+    // the time between attempts to find all peers
+    public static final Setting<TimeValue> CONSENSUS_FIND_PEERS_DELAY_SETTING =


consensus? 🗡
just FIND_PEERS_DELAY_SETTING
I also wonder if we should call this FIND_PEERS_INTERVAL_SETTING

Also, register the setting please

ywelsch · 2018-07-30T07:56:55Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeerFinder.java

+        return getFoundPeersSet();
+    }
+
+    public boolean foundQuorumFrom(VotingConfiguration votingConfiguration) {


This method is not used anywhere (also not in tests?)

ywelsch · 2018-07-30T07:58:14Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeerFinder.java

+        this.executorServiceFactory = executorServiceFactory;
+    }
+
+    public Iterable<DiscoveryNode> getFoundPeers() {


this method is not used anywhere (also not in tests?).

ywelsch · 2018-07-30T10:39:12Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeerFinder.java

+
+    private class ActivePeerFinder {
+        private boolean running;
+        private final Map<DiscoveryNode, FoundPeer> foundPeers;


can you make this package-visible? It's accessed by PeerFinder.

Yep, fixed. Is there a tool to tell you this? Also running.

ywelsch · 2018-07-30T10:44:34Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeerFinder.java

+                }
+
+                logger.trace("startProbe({}) found disconnected {}, probing again", transportAddress, cachedNode);
+                connectedNodes.remove(transportAddress, cachedNode);


should we also clean-up foundPeers?

I think not, but added a TODO for further discussion.

foundPeers is no longer a thing - we just use connectedNodes throughout.

ywelsch · 2018-07-30T10:49:33Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeersResponse.java

+import java.util.Optional;
+
+public class PeersResponse extends TransportResponse {
+    private final Optional<DiscoveryNode> masterNode;


I wonder if it would be nicer to have this as a boolean (isActiveMaster) stating whether the current node providing the peer response is an active master node, and have the list of discoveryNodes just called masterNodes.

As it is now, if you contact a follower the next step is just to contact the follower's leader. With a flag you'd go ahead and contact all the follower's peers before discovering that one of them is the leader.

In a properly-configured cluster it doesn't seem to make much difference, but if badly configured (e.g. the leader is not in all the unicast hosts lists, and there are lots of master-eligible nodes) then there's less traffic this way.

not sure I follow (no pun intended). A follower would return isActiveMaster = false + the active master as singleton in the list of masterEligibleNodes, which would lead to exact same behavior as what's currently in the PR if I understand this correctly.

We discussed on Zoom. There's no strong argument either way. I slightly prefer that as it is now there's a difference in the responses between a follower and a candidate that knows of a single node, although we don't use this difference anywhere. I also see that moving to a single list (a singleton from followers) or a boolean would remove a couple of lines of code that deal with the master node as a special case. We'll leave it as is.

ywelsch · 2018-07-30T10:50:50Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeersRequest.java

+
+public class PeersRequest extends TransportRequest {
+    private final DiscoveryNode sourceNode;
+    private final List<DiscoveryNode> discoveryNodes;


should we call this masterNodes (or masterEligibleNodes)?

Renamed to candidateNodes to reflect that they're only there if there isn't a known leader.

ywelsch · 2018-07-30T10:57:01Z

server/src/main/java/org/elasticsearch/cluster/coordination/RunnableUtils.java

+    /**
+     * Label a <code>Runnable</code>, overriding its <code>toString()</code> method.
+     */
+    public static Runnable labelRunnable(final Runnable runnable, final String label) {


this could be dangerous if an abstract runnable is passed in
add an assertion that it's not an AbstractRunnable?

Ah yes, this is bad. I just removed it.

…st pass it in at activation

…messages

DaveCTurner

Moved things to the discovery package.

DaveCTurner · 2018-08-06T08:51:10Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeerFinder.java

+ * under the License.
+ */
+
+package org.elasticsearch.cluster.coordination;


Ok, moved things around in 2013f10

ywelsch · 2018-08-06T11:19:32Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeerFinder.java


        if (active == false) {
-            logger.trace("ActivePeerFinder#handleWakeUp(): not running");
+            logger.trace("PeerFinder: not running");


do we even need to say that this is PeerFinder? We have a logger for PeerFinder that already spits out the class name (same thing for other logging occurrences in this class).

Of course. I pushed 7948268.

ywelsch

LGTM

ywelsch · 2018-08-06T11:21:38Z

server/src/main/java/org/elasticsearch/cluster/coordination/PeerFinder.java

-                active = false;
-                handleWakeUp();
-            }
+            active = false;


assert active == false?

The tests deactivate the PeerFinder at the end, regardless of whether it's active or not, and there's no particular reason to avoid double-deactivation or to make sure that each test ends with an active PeerFinder.

Also although I am pretty sure that we can't change the leader without being active, this is not something we guarantee, nor do I think we need to.

This reverts commit 7948268.

The `PeerFinder`, introduced in elastic#32246, obtains the collection of seed addresses configured by the user from a `ConfiguredHostsResolver`. In reality this collection comes from the `UnicastHostsProvider` via a slightly complicated threading model that performs the resolution of hostnames to addresses using a dedicated `ExecutorService`. This commit introduces an adapter to allow the `PeerFinder` to obtain its seed addresses in this manner.

The `PeerFinder`, introduced in elastic#32246, needs to be able to identify, and connect to, a remote master node using only its `TransportAddress`. This can be done by opening a single-channel connection to the address, performing a handshake, and only then forming a full-blown connection to the node. This change implements this logic.

The `PeerFinder`, introduced in #32246, needs to be able to identify, and connect to, a remote master node using only its `TransportAddress`. This can be done by opening a single-channel connection to the address, performing a handshake, and only then forming a full-blown connection to the node. This change implements this logic.

The `PeerFinder`, introduced in #32246, obtains the collection of seed addresses configured by the user from a `ConfiguredHostsResolver`. In reality this collection comes from the `UnicastHostsProvider` via a slightly complicated threading model that performs the resolution of hostnames to addresses using a dedicated `ExecutorService`. This commit introduces an adapter to allow the `PeerFinder` to obtain its seed addresses in this manner.

Today, CapturingTransport#createCapturingTransportService creates a transport service with a connection manager with reasonable default behaviours, but overriding this behaviour in a consumer is a litle tricky. Additionally, the default behaviour for opening a connection duplicates the content of the CapturingTransport#openConnection() method. This change removes this duplication by delegating to openConnection() and introduces overridable nodeConnected() and onSendRequest() methods so that consumers can alter this behaviour more easily. Relates elastic#32246 in which we test the mechanisms for opening connections to unknown (and possibly unreachable) nodes.

Today, CapturingTransport#createCapturingTransportService creates a transport service with a connection manager with reasonable default behaviours, but overriding this behaviour in a consumer is a litle tricky. Additionally, the default behaviour for opening a connection duplicates the content of the CapturingTransport#openConnection() method. This change removes this duplication by delegating to openConnection() and introduces overridable nodeConnected() and onSendRequest() methods so that consumers can alter this behaviour more easily. Relates #32246 in which we test the mechanisms for opening connections to unknown (and possibly unreachable) nodes.

DaveCTurner added >enhancement v7.0.0 :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. labels Jul 20, 2018

DaveCTurner requested a review from ywelsch July 20, 2018 15:33

ywelsch mentioned this pull request Jul 20, 2018

A new cluster coordination layer #32006

Closed

61 tasks

ywelsch suggested changes Jul 30, 2018

View reviewed changes

DaveCTurner added 22 commits July 31, 2018 12:56

No zen2 in request peers action

321def8

Fix up discovery.find_peers_interval setting

8e9c881

Remove unused foundQuorumFrom

c870c94

protected

686a434

Inline one-use method

83f7189

We do not probe the local address so it does not need to be provided

1525607

Fixup protected

0b4e9d1

Remove labelRunnable() and use an AbstractRunnable

d524a62

Just created this, don't need another copy

93ce793

Rename

38ef542

Rename

578a57f

Not so private

6ec315c

TODOs

a933a7f

Renaming

dddeecd

lastAcceptedNodes cannot change while the peerfinder is active, so ju…

2177d0a

…st pass it in at activation

Do not need the local node to be in foundPeers, so remove it

320ebdd

Rename discoveryNodes -> knownPeers in PeersRequest

3712208

Rename candidateNodes to knownPeers in PeersResponse too

a566ffc

Imports

5f12c03

Private class

dcfa1ca

Private

53c1dff

Add assertion of received term

0120029

DaveCTurner added 8 commits August 6, 2018 09:26

Safe to wake up peers even if already deactivated

f0e7155

Imports

c19b3df

Fix log message

2051db6

Oneliners

221253a

Add discoveryNode to PeersFinder.Peer.toString() and remove from log …

e5414a1

…messages

Remove TODO

3608505

Move PeerFinder machinery to discovery package

2013f10

Move ConfiguredHostsResolver interface into PeerFinder

8b605ad

DaveCTurner commented Aug 6, 2018

View reviewed changes

DaveCTurner added 2 commits August 6, 2018 09:56

Logger usage

9aa9003

Whitespace

b517ce9

ywelsch reviewed Aug 6, 2018

View reviewed changes

ywelsch approved these changes Aug 6, 2018

View reviewed changes

DaveCTurner added 3 commits August 6, 2018 12:34

No need to refer to class name in log messages

253a994

Can only deactivate an active PeerFinder

7948268

Revert "Can only deactivate an active PeerFinder"

b4df5f3

This reverts commit 7948268.

DaveCTurner merged commit 2176184 into elastic:zen2 Aug 6, 2018

DaveCTurner deleted the 2018-07-20-peerfinder branch August 6, 2018 14:26

DaveCTurner mentioned this pull request Aug 6, 2018

[Zen2] Add UnicastConfiguredHostsResolver #32642

Merged

DaveCTurner mentioned this pull request Aug 6, 2018

[Zen2] Add HandshakingTransportAddressConnector #32643

Merged

DaveCTurner mentioned this pull request Aug 21, 2018

Allow extension of CapturingTransport by subclasses #33012

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		public abstract class PeerFinder extends AbstractLifecycleComponent {

		public static final String REQUEST_PEERS_ACTION_NAME = "internal:discovery/zen2/requestpeers";

Conversation

DaveCTurner commented Jul 20, 2018

Uh oh!

elasticmachine commented Jul 20, 2018

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveCTurner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywelsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

DaveCTurner left a comment •

edited

Loading