fix(scheduler): default no-executors grace to executor timeout (>= heartbeat) - #2279
Conversation
| // Defaults to `executor_timeout_seconds` so the grace is always >= | ||
| // the executor heartbeat interval (see the field doc). | ||
| no_executors_grace_period_seconds: 180, |
There was a problem hiding this comment.
Should this be 180 by default, if the executor_heartbeat_interval_seconds is 60 secs by default?
There was a problem hiding this comment.
Yes. 180 is the default executor_timeout_seconds (the liveness window) giving 3× margin for a transiently-removed executor to re-register before its jobs fail
milenkovicm
left a comment
There was a problem hiding this comment.
Thanks @akshaychitneni this makes sense, one minor comment, and a question if we can add validation method now when we have dependencies between configuration values
ffb6901 to
e57e504
Compare
Thanks. Added SchedulerConfig::validate(), called from start_server. It warns when no_executors_grace_period_seconds (nonzero) is below executor_timeout_seconds, and when the timeout isn't greater than the expire interval |
…artbeat) The 30s grace could fail jobs before an executor removed by a transient launch failure (apache#2240) re-registers on its next heartbeat (~60s). Default no_executors_grace_period_seconds to executor_timeout_seconds instead, which is guaranteed longer than the heartbeat interval. Explicit values (incl. 0) still honored. Adds config tests.
e57e504 to
21dc724
Compare
milenkovicm
left a comment
There was a problem hiding this comment.
Thanks @akshaychitneni
Which issue does this PR close?
Follow-up to #2029 / #2212, addressing the timeout interaction surfaced by #2240 (#2240 (comment))
Rationale for this change
The empty-cluster grace timer defaulted to 30s, but the executor heartbeat interval defaults to 60s. Since #2240 routes task-launch-failure removals through the same ExecutorLost path (aggressive removal that also drops the heartbeat), a healthy executor removed by a transient blip re-registers only on its next heartbeat (~≤60s) — which the 30s timer could beat,
spuriously failing the job.
What changes are included in this PR?
Are there user-facing changes?
Yes — the default no-executors grace period changes from 30s to executor_timeout_seconds (180s by default); --no-executors-grace-period-seconds is now optional and defaults to that.