fix(tui): use the shared API wire types instead of local copies - #2258
Merged
Conversation
…or out dto_build Move the scheduler's REST response types into a new leaf crate, `ballista-history`, and pull the graph-to-DTO construction out of the axum handlers into a pure `api::dto_build` module. Behavior preserving: the same DTOs are produced from the same state, so live REST responses are byte-identical. The existing handler tests cover this, and the helper unit tests move alongside the functions they test. This is the first step toward a history server that replays completed jobs and serves the same `/api/*` responses without a live scheduler. Splitting the DTOs into a serde-only crate lets that server build the identical wire types without depending on the scheduler's live execution graph, and moving construction out of the handlers means it can run against state that did not come from a handler request. `JobResponse::job_id` becomes a `String` rather than `ballista_core::JobId` so the new crate stays serde-only. `JobId` is `#[serde(transparent)]` over `String`, so the JSON is unchanged.
Follow-up cleanups on the extraction: - Collapse the three near-identical ExecutionStage arms in graph_to_query_stages into one destructuring match, dropping the mutable placeholder-zero summary. - Take PlanFormat by value instead of &JobQueryParams, and move PlanFormat into ballista-history. It is part of the wire contract, and the pure builder no longer imports an axum query-param type back out of the handler module. - Inject `now` into graph_to_query_stages rather than reading the clock, so replaying a stored log renders stable elapsed times. - Share percent_complete and min_start_time; use displayable() instead of the longhand DisplayableExecutionPlan::new(). - Drop the dead JobConfig alias and the unused serde_json dev-dependency, make task_status_to_dto private, and remove a duplicated test. - Enable #![warn(missing_docs)] on ballista-history and document the types, matching the other Ballista crates. - Register ballista-history with the release tooling: version bump script, publish order, and crate dependency graph.
The previous wording left it ambiguous whether the history server re-derives responses from stored execution state or replays stored DTOs. It replays them: the scheduler builds each response once against the live graph and writes it to the event log, so byte-identical output is a structural property rather than two implementations agreeing. Also records the consequence, that anything not captured at write time cannot be recovered at replay time.
The crate holds the /api/* wire types, and it has three parties, not one: the scheduler serves them, the web TUI deserializes them, and a future history server will serve replayed copies. Naming it after the history server made it awkward for the TUI, which parses live scheduler responses and today keeps its own duplicate declarations. Renaming it after the contract it defines removes that friction. The event-log schema, writer, and reader can then land as a separate ballista-history crate that depends on this one.
The TUI declared its own structs for the scheduler's /api/* responses. Nothing kept them in step, so 2ed3464 changed TaskSummary::partition_id from u32 to Vec<u32> without touching the TUI, and the stages popup has been unable to parse a stage response since. Replace the local declarations with the shared ballista-api-types definitions, so the next scheduler-side field change is a compile error here rather than a runtime parse failure: Job -> JobResponse JobStagesResponse -> QueryStagesResponse JobStageResponse -> QueryStageSummary StageTaskResponse -> TaskSummary StageTaskStatus -> TaskStatus TaskPercentiles -> Percentiles Job's four status predicates move to a JobStatusExt extension trait, since the type now lives in another crate. Two display sites change as a result. Multi-partition tasks render the whole partition list rather than a single id, and a failed task can now show the scheduler's error text alongside the reason. Closes apache#2257.
martin-g
reviewed
Aug 10, 2026
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
Member
|
@andygrove Let me fix the build since I suggested the improvement that broke it! |
Member
|
@andygrove May I take over here ? |
martin-g
approved these changes
Aug 18, 2026
Contributor
|
thanks @andygrove & @martin-g |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #2257.
Rationale for this change
The TUI declared its own copies of the scheduler's
/api/*response types. Nothing kept the two sets in step, and they have drifted:2ed3464df(multi-partition tasks, #2038) changedTaskSummary::partition_idfromu32toVec<u32>and updatedballista-cli/src/main.rsbut not the TUI's copy, which still declaredu32. The field has no#[serde(default)], so the wholeJobStageResponsefails to deserialize and the stage detail popup breaks.A second, quieter drift in the same file:
StageTaskStatus::Failed { reason }against the scheduler'sTaskStatus::Failed { reason, error }. Serde ignores the extra field, so the TUI simply could not show the error text.The fix is not to re-sync the copies, it is to stop having copies. #2256 extracts the wire types into
ballista-api-types, a serde-only leaf crate. This PR points the TUI at them, which turns this whole class of drift into a compile error.What changes are included in this PR?
ballista-clitakes a dependency onballista-api-typesunder both thetuiandwebfeatures, and the local declarations are replaced with re-exports of the shared types under the names the TUI already used:JobJobResponseJobStagesResponseQueryStagesResponseJobStageResponseQueryStageSummaryStageTaskResponseTaskSummaryStageTaskStatusTaskStatusTaskPercentilesPercentilesJob's four status predicates (is_queued,is_running,is_completed,is_failed) become aJobStatusExtextension trait, since the type is now defined in another crate andballista-api-typesshould not carry TUI concerns.The renames the shared names imply (
stage.idtostage.stage_id, and so on) account for most of the line count. Three changes are more than mechanical:partition_idrenders as a list. Single-partition tasks look the same as before; multi-partition tasks now show every partition they own instead of failing to parse. This is the actual bug fix.Failed { reason, .. }against the richer shared enum.stage_planisOption<String>rather than a#[serde(default)]String, so the plan popup renders an empty plan as empty rather than relying on the default.start_time/end_timeareu64on the shared type where the TUI hadi64; the duration calculation usessaturating_subaccordingly, which also removes an underflow if a job ever reportsend_time < start_time.Two regression tests are added, both against payloads shaped like real scheduler output: one deserializes a task with
"partition_id": [0, 1, 2], and one deserializesFailedwith bothreasonanderror.Are there any user-facing changes?
Yes, all fixes:
No API changes.
ballista-cligains an internal dependency, andballista-api-typesis serde-only so thewasm32web build is unaffected.Verified locally:
cargo test -p ballista-cli --no-default-features --features cli,tuipasses (247 tests, including the two new ones), thewebfeature builds, and clippy is clean for both feature sets with-D warnings.