neoLsH commented on issue #12716:
URL: https://github.com/apache/gravitino/issues/12716#issuecomment-5506480358
I'd like to take this subtask. Before writing code I went through the
current job paths; summarising what I found and proposing a concrete contract,
because two decisions here change the size of the change by roughly an order of
magnitude.
### The capture half already exists
Both local process builders already redirect the child process streams to
files in the job working directory:
- `core/.../job/local/ShellProcessBuilder.java:54-57` — `output.log` /
`error.log` via `redirectOutput` / `redirectError`
- `core/.../job/local/SparkProcessBuilder.java:121-125` — same
So for the local runner this subtask is retrieval-only; nothing needs to
start capturing.
### Which means no storage-layer work
`queuedAt`/`startedAt` (#12509) needed `JobPO`, `JobEntity`, the SQL
providers and h2/mysql/postgresql schema plus upgrade scripts, because those
values are persisted. Job output is a different kind of thing: unbounded,
already on the executor's filesystem, and interesting mainly while debugging a
failed or running job. Persisting it would mean a growing text column in three
dialects and an upgrade path for data nobody asked to keep.
Proposal: fetch on demand from the executor, carry it in the response
payload, don't persist it. That keeps the change out of `JobPO`, `JobEntity`,
`JobMetaBaseSQLProvider` and `scripts/`.
The client side is consistent with this. `GenericJobHandle` is a pure
`JobDTO` holder with no `RestClient` reference, so anything exposed on
`JobHandle` has to ride in the DTO regardless — which is what "expose through
the REST `Job` payload" implies.
### Proposed contract
`JobExecutor` gains a backward-compatible default method, same shape as the
`JobHandle` ones:
```java
default JobOutputLogs retrieveJobOutputLogs(String jobId) throws
NoSuchJobException {
throw new UnsupportedOperationException(
"retrieveJobOutputLogs() is not implemented by " +
getClass().getName() + "; override this method");
}
```
stdout and stderr stay **separate**, matching the two files — merging them
discards the distinction `error.log` exists to provide.
`LocalJobExecutor` implements it. One piece of plumbing is required:
`runJob` derives the working directory inside `LocalProcessBuilder` from
`jobTemplate.executable()` and doesn't retain it, and `runningProcesses` is
cleared when the job finishes. So the executor needs to remember the resolved
working directory per `jobId`, cleaned up on the same schedule as `jobStatus`
(`jobStatusKeepTimeInMs`). Retrieval then inherits the existing lifecycle: once
an entry is cleaned, `retrieveJobOutputLogs` throws `NoSuchJobException`,
exactly as `getJobStatus` does today.
`JobManager` needs no routing logic — it holds a single `JobExecutor` from
`JobExecutorFactory.create(config)`.
### Two decisions I'd like your call on
**1. Where the payload gets populated.** The field can be filled on `GET
runs/{jobId}` only, or on every read. `GET runs` (the list endpoint) must not
fan out one filesystem read per job — on a large run list that turns a metadata
query into N executor calls. I'd populate on the single-job GET and leave the
field null in list responses, documented as such. The alternative is a
dedicated `GET runs/{jobId}/logs`, which keeps the `Job` payload clean but no
longer matches "expose it through `JobHandle`". Which do you prefer?
**2. Size bound.** `output.log` for a Spark job can be arbitrarily large,
and it would be inlined into a JSON response. I'd cap it — tail the last N
bytes per stream, N configurable, with truncation surfaced in the payload —
rather than return whole files. Is tail-with-cap acceptable, or do you want
full content?
### Scope once those are settled
`JobExecutor`, `LocalJobExecutor`, `JobManager`, `JobOperations`, `JobDTO` /
`JobHandle` / `GenericJobHandle`; on the Python side `job_handle.py`,
`generic_job_handle.py`, `job_dto.py`; `docs/open-api/jobs.yaml`; and tests
next to the existing `TestLocalJobExecutor`, `TestJobManager`,
`TestJobOperations`, `TestJobDTO` plus the Python serde and
`test_supports_jobs` tests. Same layering as #12509, minus its storage layer.
Happy to be told this is the wrong shape — the two questions above are the
only things blocking me from starting.
--
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]
