villebro commented on PR #2631: URL: https://github.com/apache/datafusion-ballista/pull/2631#issuecomment-6294445565
Thanks for the thorough read @andygrove. All fifteen are addressed in `66125eda..8c038134` (`c369693f` code, `018d364a` standalone image, `8c038134` docs), and the description is updated. Some background first, since it applies to several of the items below. The scheduler, the executors and the result service are all meant to run as several replicas eventually, so one of them going away doesn't disrupt a client. The scheduler can't yet, and this work is a step towards it: a draining scheduler fails `/readyz`, which takes it out of the client-facing Service, and the other replicas take over. The embedded proxy is what makes the scheduler a result path, and it's deprecated and removed in 57 in favour of the result service. **1–2.** I've deliberately not made the scheduler keep serving result fetches and status polls for a short window after it goes idle. With a single scheduler, stopping it takes it away from clients whatever we do, and the drain only lets running jobs finish first. A window for result fetches would postpone that by a few seconds, not avoid it. With more than one scheduler behind the Service, the per-partition connections (1) and the Flight SQL `DoGet` (2) are new connections that land on another replica. The tickets are self-contained `FetchPartition` actions, so any replica can serve them, and the result service doesn't involve the scheduler. A job's status is the exception: it lives on the scheduler that ran the job until schedulers share state, which is part of the HA work. In the meantime the docs now say that results fetched through the scheduler can be lost during a drain (`8c038134`). **3.** `serve_until_shutdown` stops the server from a drop guard, so dropping or aborting it, as `PyScheduler.close()` does, closes the connections. A test drops the future and checks that `stop` ran. **4.** Agreed, `start_server` is signal-free again. The shutdown goes in a new `start_server_with_shutdown(.., shutdown)`, which the scheduler binary and `chaos-scheduler` call with `shutdown_signal`. That leaves `start_server` with its signature, so the Python bindings and the custom-scheduler example are unchanged, and a drain timeout can't reach the bindings' `.unwrap()`. **5.** Planning runs under `catch_unwind`, and a panic fails the job through the planning-failure path, so the scheduler can still go idle. The test plans a table whose `scan` panics. **6.** The standalone entrypoint stops the scheduler first, then the executor, and exits with the scheduler's status if it failed. The guides say to stop the scheduler first under compose, which stops the executors first, and the compose scheduler gets a 30-second `stop_grace_period`. **7.** `shutdown` is now a function that is called again during the drain, and a second signal stops the server and returns an error. The result service's `serve` takes the same. **8.** Narrowed the docs and the description: `trigger_shutdown` stops the server and the dead-executor reaper. Stopping the event loop needs an `EventLoop` change (`stop()` only sets a flag, and the task holds its own sender), which I'd keep out of this PR. **9.** `admit` is `pub(crate)` again. `Admission` stays public as a field of `JobQueued`, with no public constructor. **10.** Renamed to `subscribe_for_shutdown()`. **11.** Rewrote the rolling-update paragraph: the draining pod keeps its executors, so the incoming pod isn't Ready until it exits, which takes up to the 5-second `preStop` plus the 45-second limit. **12.** Console close is documented as best-effort, because Windows ends the process about 5 seconds later. The system-shutdown claim is dropped. **13.** The OpenAPI 503 text, the `--min-ready-executors` help and `scheduler.md` now say that `/readyz` is also 503 while the scheduler is shutting down. **14.** The `active_job_number` doc now says that the step from pending to running isn't atomic, and that only the held `Admission` covers it. **15.** Both tests use `start_paused = true`. -- 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]
