andygrove opened a new pull request, #2258:
URL: https://github.com/apache/datafusion-ballista/pull/2258
# Which issue does this PR close?
Closes #2257.
> **Stacked on #2256.** GitHub shows the union of both branches until that
one merges, so review the last commit here (`fix(tui): use the shared API wire
types instead of local copies`) rather than the full diff. It needs the
`ballista-api-types` crate that #2256 introduces.
# 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) changed `TaskSummary::partition_id` from `u32`
to `Vec<u32>` and updated `ballista-cli/src/main.rs` but not the TUI's copy,
which still declared `u32`. The field has no `#[serde(default)]`, so the whole
`JobStageResponse` fails to deserialize and the stage detail popup breaks.
A second, quieter drift in the same file: `StageTaskStatus::Failed { reason
}` against the scheduler's `TaskStatus::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-cli` takes a dependency on `ballista-api-types` under both the
`tui` and `web` features, and the local declarations are replaced with
re-exports of the shared types under the names the TUI already used:
| Was (local) | Now (shared) |
| --- | --- |
| `Job` | `JobResponse` |
| `JobStagesResponse` | `QueryStagesResponse` |
| `JobStageResponse` | `QueryStageSummary` |
| `StageTaskResponse` | `TaskSummary` |
| `StageTaskStatus` | `TaskStatus` |
| `TaskPercentiles` | `Percentiles` |
`Job`'s four status predicates (`is_queued`, `is_running`, `is_completed`,
`is_failed`) become a `JobStatusExt` extension trait, since the type is now
defined in another crate and `ballista-api-types` should not carry TUI concerns.
The renames the shared names imply (`stage.id` to `stage.stage_id`, and so
on) account for most of the line count. Three changes are more than mechanical:
- **`partition_id` renders 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 tasks can show the error.** The status arm destructures `Failed {
reason, .. }` against the richer shared enum.
- **`stage_plan` is `Option<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_time` are `u64` on the shared type where the TUI had
`i64`; the duration calculation uses `saturating_sub` accordingly, which also
removes an underflow if a job ever reports `end_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 deserializes `Failed` with both `reason` and `error`.
# Are there any user-facing changes?
Yes, all fixes:
- The stage detail popup works again against a current scheduler.
- Multi-partition tasks display all their partitions instead of breaking the
response.
- Failed tasks can surface the scheduler's error text.
No API changes. `ballista-cli` gains an internal dependency, and
`ballista-api-types` is serde-only so the `wasm32` web build is unaffected.
Verified locally: `cargo test -p ballista-cli --no-default-features
--features cli,tui` passes (247 tests, including the two new ones), the `web`
feature builds, and clippy is clean for both feature sets with `-D warnings`.
--
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]
