villebro opened a new pull request, #2631: URL: https://github.com/apache/datafusion-ballista/pull/2631
## Rationale for this change With this change, stopping the scheduler no longer cuts off the requests it is serving. Until now it handled no shutdown at all: SIGTERM stopped it at once, severing in-flight `SchedulerGrpc` calls and, when the embedded Flight proxy is enabled, the result streams clients were reading through it. The executor (#2448) and the result service already drain on SIGTERM, so this brings the scheduler in line. With a single scheduler and state held only in memory, this is of limited value today: a stopped scheduler loses its jobs either way. It matters once several schedulers run (scheduler HA), because each needs to terminate cleanly, for example on node eviction or a rolling upgrade, without breaking the requests it is serving. Part of #565. ## What changes are included in this PR? - **Drain on SIGTERM or Ctrl-C.** `start_server` stops accepting connections and gives in-flight requests and proxied streams up to `--graceful-shutdown-timeout-seconds` (10 by default, like the executor and the result service) to finish. One signal drains both the gRPC service and the embedded proxy, which share a router, and stops the dead-executor reaper. - **Zero means no limit.** As for the executor, `0` waits as long as the drain takes. The result service's flag now behaves the same way; before, `0` made it give up at once. - **Shared code.** `Shutdown` and `ShutdownNotifier` move from `ballista-executor` to `ballista-core`, which the executor re-exports. `drain_or_time_out` moves to `ballista_core::utils` from the scheduler and result service, which had a copy each. - **No signature changes.** `start_grpc_service` and `start_grpc_service_with_listener` keep their signatures. The shutdown signal comes from the `SchedulerServer`. - **Config.** `SchedulerConfig` gains `graceful_shutdown_timeout_seconds` and `with_graceful_shutdown_timeout_seconds`. ## Known gaps Refusing new jobs and waiting for active jobs to finish, as #565 describes, are not included, and are planned as part of scheduler HA. This PR drains connections, so executors can't report task status during the drain and active jobs can't finish. Waiting for them takes a job-level drain, with the scheduler staying up for executors, `/readyz` reporting not-ready and new submissions refused. Refusing new jobs only helps once another scheduler can take them. ## Are there any user-facing changes? Yes, in the 56.0.0 upgrade guide: the scheduler drains on SIGTERM, and there is a new `--graceful-shutdown-timeout-seconds` flag. `SchedulerConfig` has a new public field, so a struct literal that doesn't end in `..Default::default()` needs to set it. ## How was this tested? - Unit tests for the shared `drain_or_time_out`: it gives up at its bound, passes results and errors through, and waits for a zero bound. - A unit test that axum's graceful shutdown blocks on a stalled request, which is why the bound exists. - A unit test that `trigger_shutdown()` stops the dead-executor reaper. - A chaos test, `scheduler_final_results_survive_sigterm`, SIGTERMs the scheduler while a client is mid-fetch of a large result through the embedded proxy. It checks that the result isn't truncated and the scheduler exits cleanly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
