neoLsH commented on PR #13250: URL: https://github.com/apache/gravitino/pull/13250#issuecomment-5712483618
@jerryshao thanks — I read the PR end to end. The shape did converge: fetch-on-demand rather than persisted, capped and tailed, exposed through the existing `getJob` path, all match what I'd sketched. You asked where it diverges from what I had in mind, so here are the places, and they're design trade-offs rather than bugs — I'll anchor each to the decision in my write-up it came from. The one I'd most want to reconcile is the cap: I bounded bytes, the PR bounds lines. My sketch had `gravitino.jobExecutor.local.outputLogMaxBytes`, 256 KiB per stream, read via `RandomAccessFile.seek` cut on a UTF-8 boundary with a `truncated` flag, precisely because a runaway job can produce gigabytes and a byte bound holds during the read. Moving the cap into `Configs` so every executor honors one policy is a better call than my executor-local key — I'd take that part. But lines don't bound bytes, and `ReversedLinesFileReader` degrades hard when there's no separator to find: `createLeftOver()` copies the whole accumulated buffer into the next `FilePart`, so a single line of S bytes costs roughly S²/(2·blockSize) of copying with blockSize around 4096. A few hundred MB with no line breaks — `base64 -w0`, a minified JSON dump, `cat` of a binary — hangs the JAX-RS request thread for minutes, and a GB-scale one OOMs before returning; `outputMaxLines` only checks `value > 0`, so nothing else catches it. My instinct is still a byte bound applied while reading rather than a line count applied after. That's also why the `truncated` flag disappeared, and it's really a second divergence — the SPI shape. I'd sketched one `retrieveJobOutputLogs(jobExecutionId)` returning a `JobOutputLogs(stdout, stderr, truncated)`; the PR has `getJobStdout(id, maxLines)` and `getJobStderr(id, maxLines)` each returning `List<String>`. Two calls means two reverse reads now and two round trips for a future remote executor, with the two streams snapshotted at different moments for a job that's still running. A single value object would make it one atomic fetch and give `truncated` a natural home, so a caller getting exactly 1000 lines could tell "the job printed 1000" from "we cut it off" — which matters because a tail can hide the line that actually explains a failure. The retention window one's on me. In my write-up I argued this was "safe without an expiry state on the wire" because `cleanUpStagingDirs()` drops the entity and the staging dir in the same 7-day pass, so "there's no window where getJob succeeds but the logs are gone." But I also said `jobWorkingDirs` evicts on `jobStatusKeepTimeInMs`, and those only reconcile if the two keep-times are equal — they aren't by default, 1 hour vs 7 days. The PR faithfully implements the evict-on-`jobStatusKeepTimeInMs` half, so the window I claimed didn't exist is real: for about six days and twenty-three hours the `output.log` is on disk and the job is queryable, but `includeOutput=true` comes back empty. So my question is scope — is this meant as a best-effort live tail of a running or just-finished job, or should it answer "a job failed yesterday, why"? The second was the use case I had in mind. And one neither of us covered: multi-node. `jobWorkingDirs` is a per-node retained map, and the new fetch at `JobManager.java:456-457` doesn't gate on `ownsJob` the way the status pull does at 701, so a node that didn't run the job misses, `getWorkingDir` throws, and the catch degrades to empty — a 200 with no logs, and not transient, since behind a load balancer a non-owner node returns empty every time. I used the same retained map in my sketch, so it's a shared blind spot rather than something the PR introduced. The 1-hour window, the restart loss and this all trace to one root: the working dir is retained rather than derived, and `JobManager` can derive it — `cleanUpStagingDirs` already recomputes the path in three places. I still think keying the SPI on `jobExecutionId` is defensible, since passing a directory in would leak local-runner assumptions into an interface a remote Airflow or Livy executor also implements, so I'd want the consequence surfaced rather than silentl y degraded, not a redesign. None of this blocks the happy path, which works and is well tested, and I'm glad to take any of it as follow-ups. The byte-vs-line cap and the retention scope are the two I'd most want your read on before merge. -- 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]
