andygrove opened a new pull request, #2264:
URL: https://github.com/apache/datafusion-ballista/pull/2264
# Which issue does this PR close?
Part of #1923. Third of four PRs splitting up #1925, after #2256 (merged)
and #2260.
> **Stacked on #2260.** GitHub shows both branches until that one merges, so
review the last commit here (`feat(scheduler): record a per-job event log
behind --event-log-dir`). It needs the `ballista-history` crate #2260
introduces.
# Rationale for this change
#2260 added the event-log format and the machinery to read and write it, but
nothing produces one. This wires it into the scheduler so a running cluster
actually emits logs. Nothing reads them yet: the history server that serves
them is the last slice.
Splitting it out this way keeps the reviewable question narrow. This is the
only slice that touches the scheduler's event loop, so it is the one where the
cost of being wrong is a scheduling regression rather than a missing feature.
That deserves its own read, rather than being buried in a PR that also
introduces a binary and a docs page.
# What changes are included in this PR?
**A new `--event-log-dir` flag, off by default.** When it is unset there is
no channel, no background task, no file, and no per-event work beyond one
`Option` check in `on_receive`. A cluster that does not opt in is unaffected.
**`scheduler_server/event_log.rs`** builds `HistoryEvent`s from
execution-graph state. It reuses `api::dto_build`, the same builders backing
the live REST API, so a job's stored record and its `GET /api/job/{id}`
response are the same bytes for the same graph. That reuse is the whole reason
#2256 extracted those builders out of the axum handlers.
**A tee at the top of `QueryStageScheduler::on_receive`** maps
`JobSubmitted`, `TaskUpdating`, `JobFinished`, `JobRunningFailed` and
`JobCancel` onto history events.
Three decisions in there worth review attention:
- **`JobCancel` is handled in the tee** rather than alongside the other
terminal events, because the handler below it drops the graph. That makes the
tee the last point at which a cancelled job can be recorded at all. Without it,
a cancelled job would never get a terminal record, leaving it invisible to the
history server and its file handle open for the life of the process.
- **A cancelled job's status is overridden on the typed DTO before
serialization**, not by rewriting the stored JSON afterwards. The tee runs
before the scheduler applies the cancel, so the graph still reports the job as
`Running`. Patching the serialized payload would have worked, but it would also
have broken the property that a stored payload is a faithful serialization of
exactly one value, which is what lets the history server relay it verbatim.
- **`JobPlanningFailed` is deliberately absent.** It is posted instead of
`JobSubmitted` when `submit_job` fails, so the job has neither an execution
graph nor an open log file. There is nothing to record and nothing to close.
**Event logging never fails a job.** A missing execution graph or a
serialization error is logged and skipped. The worst case is a job with no
record, never a job that does not run.
**The stage snapshot embedded in `JobEnd` is rendered as of
`completed_at`**, not the wall clock, so replaying a log is deterministic
rather than dependent on when the writer happened to run.
# Are there any user-facing changes?
One new opt-in flag, `--event-log-dir <dir>`, disabled by default. No
behaviour change for clusters that do not set it, no API changes, no
configuration defaults altered.
Verified locally: `cargo test -p ballista-scheduler --lib` passes (331
tests, 7 of them new), clippy is clean with `--all-features -D warnings`, fmt
and taplo are clean, and the `--no-default-features` check CI runs still passes.
# What is not here
The end-to-end guarantee.
`history_store_serves_byte_identical_json_to_live_scheduler` asserts that the
history server serves exactly what the live scheduler would for the same graph,
but it needs `HistoryStore`, which arrives with the server in the next slice.
It moves there rather than being duplicated or weakened here.
So this slice lands with the emission path covered by unit tests on the
event builders and a round trip through the real async writer, but the parity
claim itself is proved one PR later. Naming that explicitly because it is the
one place the split is not clean.
# Follow-up
4. The history server binary, its docs, and the byte-identity test.
--
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]
