andygrove opened a new pull request, #2256: URL: https://github.com/apache/datafusion-ballista/pull/2256
# Which issue does this PR close? Part of #1923. This is the first of four PRs splitting up #1925, which is too large to review as one change. # Rationale for this change Ballista's live TUI reads the scheduler's REST API, but that state is ephemeral: completed jobs are cleaned up after `finished_job_state_clean_up_interval_seconds`, and everything is gone when the scheduler restarts. #1923 asks for a Spark History Server equivalent, where a standalone server replays durable event logs and serves the same `/api/*` responses so the existing TUI can browse completed jobs with no scheduler running. Two things have to be true for that to work, and both are refactors rather than features: 1. The REST response types have to live somewhere the history server can reach without depending on the scheduler's live execution graph. Today they are private types inside `ballista-scheduler`. 2. DTO construction has to be callable from outside an axum handler. Today it is inlined in the handler bodies, so the only way to produce a `JobResponse` is to serve an HTTP request. This PR does both, and nothing else. No new feature, no new config, no behavior change. Landing it separately keeps the reviewable question down to one thing: is this behavior preserving? # What changes are included in this PR? **New `ballista-history` crate.** A leaf crate whose only dependency is `serde`. It holds the REST response types shared by the live scheduler and, later, the history server: `JobResponse`, `TaskSummary`, `TaskStatus`, `Percentiles`, `QueryStageSummary`, `QueryStagesResponse`, and a `JobConfig` alias. Types that depend on live scheduler state or on `ballista-core` (`ExecutorResponse`, `ExecutorMetricResponse`, `SchedulerStateResponse`, `SchedulerVersionResponse`, `CancelJobResponse`) deliberately stay in `handlers.rs`. They are not replayable from a stored log, so there is nothing to share. **New `api::dto_build` module.** The graph-to-DTO translation moves out of the handler bodies into pure functions: `job_overview_to_response`, `graph_to_job_response`, and `graph_to_query_stages`, plus the formatting and percentile helpers they use. State in, DTO out, no I/O. `handlers.rs` drops from 1427 lines to 768 and is now mostly HTTP concerns. **Slight dedup along the way.** The three `ExecutionStage` arms in `get_query_stages` had three near-identical copies of the per-task summary loop and the plan-rendering match; those become `task_summaries` and `render_stage_plan`. The two percentile functions shared a body, which becomes `percentiles_of`. These are the only structural changes, and they are all local. **One type change.** `JobResponse::job_id` is a `String` rather than `ballista_core::JobId`, so the new crate can stay serde-only. `JobId` is `#[serde(transparent)]` over `String`, so the serialized JSON is identical. **`impl From<&task_status::Status> for TaskStatus` becomes the free function `task_status_to_dto`.** Both types are now foreign to `ballista-scheduler`, so the impl would violate the orphan rule. The 13 helper unit tests move to `dto_build.rs` alongside the functions they cover. The 8 `get_webtui` tests stay in `handlers.rs`. # Are there any user-facing changes? No. REST responses are byte-identical, no public API of `ballista-scheduler` changes, and no configuration is added. `ballista-history` is new but nothing outside the scheduler depends on it yet. Verified locally: `cargo test -p ballista-scheduler --lib` passes (325 tests), clippy is clean for `ballista-scheduler` and `ballista-history` with `--all-features -D warnings`, `cargo fmt --all --check` and taplo are clean, and the `--no-default-features` check CI runs still passes. # Follow-ups The remaining three slices of #1925, in order: 2. Event schema, writer, and reader in `ballista-history` (no scheduler changes). 3. Scheduler event-log wiring behind a new `event_log_dir` config, off by default. 4. The history server itself, its binary, docs, and the byte-identical-JSON end-to-end test. # Open questions for reviewers Two decisions I made by default and would happily change: - `ballista-history` has no `publish = false`, so it would be published on the next release. That is a permanent commitment for what is currently an internal detail. Worth deciding deliberately. - The crate does not set `#![warn(missing_docs)]`, unlike `core` / `scheduler` / `executor` / `client`. Adding it means documenting every DTO field, which felt out of scope for a move, but I am happy to do it here rather than later. -- 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]
