[Zen2] Introduce ElectionScheduler #32846

DaveCTurner · 2018-08-14T14:17:08Z

The ElectionScheduler runs while there is no known elected master and is
responsible for scheduling elections randomly, backing off on failure, to
balance the desire to elect a master quickly with the desire to avoid more than
one node starting an election at once.

The ElectionScheduler runs while there is no known elected master and is responsible for scheduling elections randomly, backing off on failure, to balance the desire to elect a master quickly with the desire to avoid more than one node starting an election at once.

ywelsch

I've left some smaller comments. Main change looks good though.

ywelsch · 2018-08-14T14:23:14Z

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

+    @Nullable
+    private volatile Object currentScheduler; // only care about its identity; null if stopped
+
+    ElectionScheduler(Settings settings, Random random, ThreadPool threadPool) {


make this public please.

ywelsch · 2018-08-14T14:40:23Z

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

+
+public class ElectionScheduler extends AbstractComponent {
+
+    /*


move this as Javadoc to the class level?

ywelsch · 2018-08-14T14:43:52Z

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

+    /**
+     * @param upperBound exclusive upper bound
+     */
+    private long randomPositiveLongLessThan(long upperBound) {


make this a static method (random instance as parameter) so that it can be unittested in isolation.

Ok. And I added tests.

ywelsch · 2018-08-14T14:45:19Z

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

+        return nonNegative(random.nextLong()) % (upperBound - 1) + 1;
+    }
+
+    private long backOffCurrentMaxDelay(long currentMaxDelayMillis) {


Should we call this boundDelayByMaxTimeout? In any case, please add a comment what this method does

Ok, more docs.

ywelsch · 2018-08-14T14:53:04Z

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

+                        logger.debug("{} not starting election", ElectionScheduler.this);
+                        return;
+                    }
+                    logger.debug("{} starting pre-voting", ElectionScheduler.this);


Maybe a more generic message here? Something like executing scheduled election. The Prevoting class can output it's own log line that prevoting has started.

ywelsch · 2018-08-14T14:56:51Z

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

+
+    private class ElectionScheduler implements Releasable {
+        private AtomicLong currentMaxDelayMillis = new AtomicLong(minTimeout.millis());
+        private AtomicBoolean isRunning = new AtomicBoolean(true);


also, change it to isClosed (or just closed), so that we don't have to write == false everywhere :)

ywelsch · 2018-08-14T15:36:12Z

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

+        private AtomicBoolean isRunning = new AtomicBoolean(true);
+
+        void scheduleNextElection(final TimeValue gracePeriod, final Runnable scheduledRunnable) {
+            final long delay;


while declare it here? Just declare where it's defined

Hangover from an earlier implementation, now fixed.

ywelsch · 2018-08-14T15:40:57Z

server/src/test/java/org/elasticsearch/cluster/coordination/ElectionSchedulerFactoryTests.java

+
+    private DeterministicTaskQueue deterministicTaskQueue;
+    private ElectionSchedulerFactory electionSchedulerFactory;
+    private boolean electionStarted = false;


does this need to live at the class level? can we move this to the test method? Same for electionSchedulerFactory

Ok, only used by one test now, so can be local to that (with suitable parameters)

ywelsch · 2018-08-14T16:09:48Z

server/src/test/java/org/elasticsearch/cluster/coordination/ElectionSchedulerFactoryTests.java

+                    assertThat(electionDelay, greaterThanOrEqualTo(initialGracePeriod.millis()));
+
+                    // Check upper bound
+                    assertThat(electionDelay, lessThanOrEqualTo(ELECTION_MIN_TIMEOUT_SETTING.get(Settings.EMPTY).millis()


should we inject random settings with min timeout/ max timeout/backoff to test this on different settings?

Ok. This was a little painful because putting a TimeValue into a Settings.Builder converts it (lossily) to a string and then back again, and even rejects things with a fraction like 1.1m. Putting a specific number of milliseconds in as a string is the answer.

ywelsch · 2018-08-14T16:12:45Z

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

+
+    // bounds on the time between election attempts
+    private static final String ELECTION_MIN_TIMEOUT_SETTING_KEY = "cluster.election.min_timeout";
+    private static final String ELECTION_BACK_OFF_TIME_SETTING_KEY = "cluster.election.back_off_time";


can you document how the backoff works, i.e. the initial interval, then the subsequent intervals we consider.

ywelsch

I've left some more comments, mostly very minor stuff, and left a few suggestions which I would like to get your opinion on.

ywelsch · 2018-08-15T10:59:42Z

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

+     * The first election is scheduled to occur a random number of milliseconds after the scheduler is started, where the random number of
+     * milliseconds is chosen uniformly from
+     *
+     *     (0, min(ELECTION_INITIAL_TIMEOUT_SETTING, ELECTION_MAX_TIMEOUT_SETTING_KEY)]


remove _KEY

ywelsch · 2018-08-15T11:01:40Z

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

+     * For `n > 1`, the `n`th election is scheduled to occur a random number of milliseconds after the `n - 1`th election, where the random
+     * number of milliseconds is chosen uniformly from
+     *
+     *     (0, min(ELECTION_INITIAL_TIMEOUT_SETTING + (n-1) * ELECTION_BACK_OFF_TIME_SETTING, ELECTION_MAX_TIMEOUT_SETTING_KEY)]


remove _KEY

ywelsch · 2018-08-15T11:04:46Z

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

+     * @param scheduledRunnable The action to run each time an election should be attempted.
+     */
+    public Releasable startElectionScheduler(TimeValue gracePeriod, Runnable scheduledRunnable) {
+        final ElectionScheduler currentScheduler = new ElectionScheduler();


just call this scheduler

ywelsch · 2018-08-15T11:07:58Z

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

+
+    /**
+     * @param randomSupplier supplier of randomly-chosen longs
+     * @param upperBound     exclusive upper bound


also document @return here, to denote what's returned? This will go nicely with your explanation above about the uniformly chosen value.

😐 it returns a random positive long that's less than upperBound.

(actually neater to make it an inclusive upper bound now - this is on it way)

ywelsch · 2018-08-15T11:11:03Z

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

+         * The current maximum timeout: the next election is scheduled randomly no later than this number of milliseconds in the future. On
+         * each election attempt this value is increased by `backoffTime`, up to the `maxTimeout`, to adapt to higher-than-expected latency.
+         */
+        private final AtomicLong currentMaxTimeoutMillis = new AtomicLong(Math.min(initialTimeout.millis(), maxTimeout.millis()));


why not enforce the condition on maxTimeout to be >= initialTimeout?

If maxTimeout < initialTimeout then nothing particularly bad happens. I added this call to Math.min because the spec is more uniform with it there than not.

sure, nothing bad happens, but it might still be a user configuration error (user configured initalTimeout, but forgot to set maxTimeout). I think it's nicer to just throw an exception and report it.

ywelsch · 2018-08-15T11:15:58Z

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

+                public boolean isForceExecution() {
+                    // There are very few of these scheduled, and they back off, but it's important that they're not rejected as
+                    // this could prevent a cluster from ever forming.
+                    return true;


still not sure this is needed. Have you checked this? If not, let's add a TODO here.

ThreadPool#schedule() wraps this up in a ThreadedRunnable, but I think that's what we want. When the ThreadedRunnable finally runs, it runs this AbstractRunnable directly using the executor, so rejections seem to be handled right.

As far as I can see there's nothing special about whether the generic threadpool does rejections, but perhaps I'm missing something.

The generic threadpool has an unbounded queue, so rejections should not happen unless the executor is shutdown. In that case, there's no need for this code to still execute.

ywelsch · 2018-08-15T11:17:23Z

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

+
+        @Override
+        public void close() {
+            boolean isClosedChanged = isClosed.compareAndSet(false, true);


maybe call this previouslyNotClosed

ywelsch · 2018-08-15T11:26:56Z

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

+     * For `n > 1`, the `n`th election is scheduled to occur a random number of milliseconds after the `n - 1`th election, where the random
+     * number of milliseconds is chosen uniformly from
+     *
+     *     (0, min(ELECTION_INITIAL_TIMEOUT_SETTING + (n-1) * ELECTION_BACK_OFF_TIME_SETTING, ELECTION_MAX_TIMEOUT_SETTING_KEY)]


I like this formula. I wonder if we should try to implement ElectionScheduler exactly this way. We would "just" need to keep track of the round n (which might be useful anyhow for debugging purposes), and there would be no need for currentMaxTimeoutMillis, which feels more awkward. The only arithmetic calculation we would need to do then would be just a straight-forward implementation of this formula, which would preferably be done in a single static method at the top-level here. This would also allow us to get rid of the methods randomPositiveLongLessThan and backOffCurrentMaxTimeout, both of which could be folded into this static method.

Hm. I did this, but there's now potential for integer overflow, so perhaps we do need that upper bound on the settings?

Also I'm unconvinced at the need for a static method to do this calculation. We test it via the actual timeouts chosen, so why extract it?

Also randomPositiveLongAtMost is still needed, and you wanted a test for that.

On reflection, there was already potential for integer overflow without an upper bound on the settings and this change makes it no worse.

DaveCTurner

There were two remaining discussion points in #32709 - I brought them over here.

DaveCTurner · 2018-08-15T11:39:16Z

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

+    public static final Setting<TimeValue> ELECTION_BACK_OFF_TIME_SETTING = Setting.timeSetting(ELECTION_BACK_OFF_TIME_SETTING_KEY,
+        TimeValue.timeValueMillis(100), TimeValue.timeValueMillis(1), Property.NodeScope);
+
+    public static final Setting<TimeValue> ELECTION_MAX_TIMEOUT_SETTING = Setting.timeSetting(ELECTION_MAX_TIMEOUT_SETTING_KEY,


@ywelsch you asked for a Setting.timeSetting() that allowed us to specify an upper bound for these settings as well as a lower bound. I don't think we need this - the consequences of setting ELECTION_MAX_TIMEOUT_SETTING too high do not seem terribly surprising, and documenting this seems sufficient.

DaveCTurner · 2018-08-15T11:42:52Z

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

+                }
+
+                @Override
+                public boolean isForceExecution() {


@ywelsch I didn't totally understand what action you wanted here (see
#32709 (comment)).

ywelsch · 2018-08-15T16:21:12Z

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

+                public boolean isForceExecution() {
+                    // There are very few of these scheduled, and they back off, but it's important that they're not rejected as
+                    // this could prevent a cluster from ever forming.
+                    return true;


The generic threadpool has an unbounded queue, so rejections should not happen unless the executor is shutdown. In that case, there's no need for this code to still execute.

ywelsch · 2018-08-15T16:24:13Z

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

+            }
+
+            final long thisAttempt = attempt.getAndIncrement();
+            final long maxDelayMillis = Math.min(maxTimeout.millis(), initialTimeout.millis() + thisAttempt * backoffTime.millis());


You mentioned the overflow that could happen here. Can you please add a safeguard? Otherwise I might not be able to sleep anymore 😨

Ok. It's a bit daft because now that there are upper limits on the settings it would take solidly more than a million years to overflow, but there you go.

if you put it like that, you can undo c08cd8b

ywelsch · 2018-08-15T16:28:01Z

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

+         * The current maximum timeout: the next election is scheduled randomly no later than this number of milliseconds in the future. On
+         * each election attempt this value is increased by `backoffTime`, up to the `maxTimeout`, to adapt to higher-than-expected latency.
+         */
+        private final AtomicLong currentMaxTimeoutMillis = new AtomicLong(Math.min(initialTimeout.millis(), maxTimeout.millis()));


sure, nothing bad happens, but it might still be a user configuration error (user configured initalTimeout, but forgot to set maxTimeout). I think it's nicer to just throw an exception and report it.

ywelsch

LGTM (two nits)

This reverts commit c08cd8b.

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

DaveCTurner requested a review from ywelsch August 14, 2018 14:17

DaveCTurner changed the title ~~Introduce ElectionScheduler~~ [Zen2] Introduce ElectionScheduler Aug 14, 2018

DaveCTurner mentioned this pull request Aug 14, 2018

[Zen2] Introduce ElectionScheduler #32709

Closed

ywelsch mentioned this pull request Aug 14, 2018

A new cluster coordination layer #32006

Closed

61 tasks

Factory instead

1f3ae24

ywelsch suggested changes Aug 14, 2018

View reviewed changes

DaveCTurner added 11 commits August 15, 2018 09:59

Move comment to javadoc

47bb992

public

498c322

Add test case for randomPositiveLongLessThan

038a4d1

Rename MIN to INITIAL timeout, add explanatory comments

771ac3e

debug message

ddf46c1

Improve toString()

ea57415

final

705781c

isRunning -> isClosed

fcf1070

Docs and allow max < initial

87f8cba

Move suite-level state into the test

b445398

Test behaviour with random settings too

cd0d527

DaveCTurner requested a review from ywelsch August 15, 2018 11:29

ywelsch reviewed Aug 15, 2018

View reviewed changes

DaveCTurner commented Aug 15, 2018

View reviewed changes

DaveCTurner added 4 commits August 15, 2018 12:47

Review comments

42de1f6

Rename

447b66b

Track attempt count rather than currentMaxTimeoutMillis

3fe405d

Apply upper bounds to settings

722fd54

ywelsch reviewed Aug 15, 2018

View reviewed changes

ywelsch approved these changes Aug 15, 2018

View reviewed changes

DaveCTurner added 6 commits August 15, 2018 17:57

No need for forcing execution

51f85b7

Fix bounds on settings set in tests

a72531b

Protect against overflow

c08cd8b

Validate that max timeout is >= initial timeout

c9de21d

Revert "Protect against overflow"

56dee44

This reverts commit c08cd8b.

Note overflow consideration in comment

548fdbc

DaveCTurner merged commit 6d9e7c5 into elastic:zen2 Aug 15, 2018

DaveCTurner deleted the 2018-08-14-election-scheduler branch August 15, 2018 19:48

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


		public class ElectionScheduler extends AbstractComponent {

		/*

[Zen2] Introduce ElectionScheduler #32846

[Zen2] Introduce ElectionScheduler #32846

Conversation

DaveCTurner commented Aug 14, 2018

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch left a comment

Choose a reason for hiding this comment