[Zen2] Introduce ElectionScheduler #32709
Conversation
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.
DaveCTurner
added
>enhancement
v7.0.0
:Distributed Coordination/Cluster Coordination
|
Pinging @elastic/es-distributed |
|
Marking this as WIP because I realised I want it to be restartable, and currently it's not. |
…so can be removed
…fies things a bit
| reasonableTimeParser(ELECTION_MAX_RETRY_INTERVAL_SETTING_KEY), | ||
| new ElectionMaxRetryIntervalSettingValidator(), Property.NodeScope, Property.Dynamic); | ||
|
|
||
| private static Function<String, TimeValue> reasonableTimeParser(final String settingKey) { |
There was a problem hiding this comment.
let's add a parseTimeValue method to Setting that allows to specify both a min and a max.
| } | ||
|
|
||
| static void validateSettings(final TimeValue electionMinRetryInterval, final TimeValue electionMaxRetryInterval) { | ||
| if (electionMaxRetryInterval.millis() < electionMinRetryInterval.millis() + 100) { |
There was a problem hiding this comment.
I wonder if we should instead limit electionMaxRetryInterval to minimum 100ms and check that min <= max.
There was a problem hiding this comment.
I think it's important that there's some margin between min and max. If they're equal then there's a chance that two nodes end up in lockstep, repeatedly interfering with each other's elections.
| 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; |
There was a problem hiding this comment.
The generic threadpool never rejects anyone. I wonder though if the ScheduledThreadPoolExecutor can reject us. As ThreadPool.schedule wraps the executable in an ThreadedRunnable, which is not an AbstractRunnable, this might cause problems? ScheduledThreadPoolExecutor looks to have an unbounded queue though, so needs more investigation...
| } | ||
| } | ||
|
|
||
| public void start() { |
There was a problem hiding this comment.
maybe use the activate / deactivate terminology so that this is not confused with the lifecycle of AbstractLifeCycleComponent
| private static final String ELECTION_MAX_RETRY_INTERVAL_SETTING_KEY = "discovery.election.max_retry_interval"; | ||
|
|
||
| public static final Setting<TimeValue> ELECTION_MIN_RETRY_INTERVAL_SETTING | ||
| = new Setting<>(ELECTION_MIN_RETRY_INTERVAL_SETTING_KEY, "300ms", |
There was a problem hiding this comment.
300ms is quite long. I wonder if we should go with something a bit more optimistic, say 100ms
There was a problem hiding this comment.
I reduced to 100ms. There's opportunity to debate how we calculate the actual delays between election attempts. Currently they're never less than 100ms (previously 300ms) apart but in fact I can't think of a good reason for this lower bound.
|
|
||
| delay = randomLongBetween(electionMinRetryInterval.getMillis(), currentDelayMillis + 1); | ||
| } | ||
| logger.trace("scheduling election after delay of [{}ms]", delay); |
There was a problem hiding this comment.
maybe with delay instead of after delay (after sounds as if the delay has already passed)
| return; | ||
| } | ||
| } | ||
| startElection(); |
There was a problem hiding this comment.
add trace logging here that election has started
|
|
||
| // bounds on the time between election attempts | ||
| private static final String ELECTION_MIN_RETRY_INTERVAL_SETTING_KEY = "discovery.election.min_retry_interval"; | ||
| private static final String ELECTION_MAX_RETRY_INTERVAL_SETTING_KEY = "discovery.election.max_retry_interval"; |
There was a problem hiding this comment.
I think we should go with naming that's more established in the literature, and use something like min_election_timeout and max_election_timeout
There was a problem hiding this comment.
Ok. I don't particularly like the "timeout" nomenclature - "timeout" suggests how long some process has before it's considered to have failed, but here we're counting down until something starts. But I can live with it.
|
|
||
| @Override | ||
| public String toString() { | ||
| return "ElectionScheduler: do election and schedule retry"; |
There was a problem hiding this comment.
maybe we could put the delay in here as well
|
|
||
| delay = randomLongBetween(electionMinRetryInterval.getMillis(), currentDelayMillis + 1); | ||
| } | ||
| logger.trace("scheduling election after delay of [{}ms]", delay); |
There was a problem hiding this comment.
let's also log the currentDelayMillis, min and max here
|
|
||
| // bounds on the time between election attempts | ||
| private static final String ELECTION_MIN_RETRY_INTERVAL_SETTING_KEY = "discovery.election.min_retry_interval"; | ||
| private static final String ELECTION_MAX_RETRY_INTERVAL_SETTING_KEY = "discovery.election.max_retry_interval"; |
There was a problem hiding this comment.
Ok. I don't particularly like the "timeout" nomenclature - "timeout" suggests how long some process has before it's considered to have failed, but here we're counting down until something starts. But I can live with it.
| private static final String ELECTION_MAX_RETRY_INTERVAL_SETTING_KEY = "discovery.election.max_retry_interval"; | ||
|
|
||
| public static final Setting<TimeValue> ELECTION_MIN_RETRY_INTERVAL_SETTING | ||
| = new Setting<>(ELECTION_MIN_RETRY_INTERVAL_SETTING_KEY, "300ms", |
There was a problem hiding this comment.
I reduced to 100ms. There's opportunity to debate how we calculate the actual delays between election attempts. Currently they're never less than 100ms (previously 300ms) apart but in fact I can't think of a good reason for this lower bound.
| } | ||
|
|
||
| static void validateSettings(final TimeValue electionMinRetryInterval, final TimeValue electionMaxRetryInterval) { | ||
| if (electionMaxRetryInterval.millis() < electionMinRetryInterval.millis() + 100) { |
There was a problem hiding this comment.
I think it's important that there's some margin between min and max. If they're equal then there's a chance that two nodes end up in lockstep, repeatedly interfering with each other's elections.
| return this == currentScheduler; | ||
| } | ||
|
|
||
| private void scheduleNextElection() { |
There was a problem hiding this comment.
Yes, that's nicer.
| * It's provably impossible to guarantee that any leader election algorithm ever elects a leader, but they generally work (with | ||
| * probability that approaches 1 over time) as long as elections occur sufficiently infrequently, compared to the time it takes to send | ||
| * a message to another node and receive a response back. We do not know the round-trip latency here, but we can approximate it by | ||
| * attempting elections randomly at reasonably high frequency and backing off (linearly) until one of them succeeds. We also place an |
There was a problem hiding this comment.
Yes, I've added such a setting.
| */ | ||
|
|
||
| // bounds on the time between election attempts | ||
| private static final String ELECTION_MIN_RETRY_INTERVAL_SETTING_KEY = "discovery.election.min_retry_interval"; |
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.