jerryshao opened a new pull request, #13373:
URL: https://github.com/apache/gravitino/pull/13373

   > **Depends on #13250.** This PR is stacked on #13250, so its diff against 
`main` still contains #13250's commit. To review only this PR's changes, see 
the [compare 
view](https://github.com/jerryshao/gravitino/compare/claude/github-issue-review-441d59...claude/job-output-multi-node).
 It stays a draft until #13250 merges, and will then be rebased onto `main` and 
marked ready for review.
   
   ### What changes were proposed in this pull request?
   
   Follow-up to #13250, for the multi-node gap raised in its review. With the 
local job executor, `getJob(includeOutput=true)` only returned a job's output 
on the server that ran it.
   
   - `LocalJobExecutor` no longer keeps job working directories in an in-memory 
map. When a job is submitted, it writes a write-once index file, 
`<gravitino.job.stagingDir>/.job-output-index/<jobId>.json`, containing 
`{"version":1,"workingDir":"<path relative to the staging directory>"}`. The 
output is found from the job id alone, so any server sharing the staging 
directory can return it. This also works after a restart or a template rename.
   - The output files stay where they were (`output.log` / `error.log` in the 
job's staging directory). The `JobExecutor` SPI and the `JobManager` logic are 
unchanged.
   - `JobExecutorFactory` passes `gravitino.job.stagingDir` to the built-in 
local job executor.
   - An hourly task removes index files that are more than an hour old and 
whose staging directory `JobManager` has already removed. An index therefore 
lives exactly as long as its job's output.
   - Output retrieval stays best-effort:
     - Failing to create or write the index only logs a warning. It never fails 
a job submission or the server startup.
     - An invalid index, or one pointing outside the staging directory, returns 
empty output.
   - Docs:
     - the shared staging directory requirement;
     - the index directory and the upgrade/downgrade behavior;
     - removed the old statement that output is lost on restart.
   
   ### Why are the changes needed?
   
   In a multi-server deployment behind a load balancer, a request for a job's 
output returned empty output whenever it reached a server other than the one 
that ran the job. That looks exactly like a job that printed nothing. The same 
happened on the server that ran the job, once the executor's in-memory status 
expired (after 1 hour) or after a restart, even though the staging directory is 
kept for `gravitino.job.stagingDirKeepTimeInMs` (7 days by default).
   
   Related: #12716 (follow-up to #13250), epic #12667. Pre-existing issues 
found along the way: #13371, #13372.
   
   ### Does this PR introduce _any_ user-facing change?
   
   - `getJob(includeOutput=true)` returns the output from any server sharing 
`gravitino.job.stagingDir`, also after server restarts, until the job's staging 
directory is removed.
   - A new directory, `<gravitino.job.stagingDir>/.job-output-index`. No 
configuration property is added or changed.
   - Jobs submitted before this change, or by a server not yet upgraded during 
a rolling upgrade, return empty output.
   - Servers that don't share the staging directory still only return output 
for the jobs they ran. This is documented, and they now log a warning.
   
   ### How was this patch tested?
   
   - `TestLocalJobExecutor`:
     - the index content;
     - reading the output from another executor instance, and after the job 
status expired;
     - special characters in the working directory;
     - invalid index contents and job ids;
     - the cleanup: age guard, interruption, missing directory;
     - index write failures, and concurrent initialization;
     - warning only once.
   - `TestJobManagerMultiNode`, reading a job's output:
     - from another node;
     - after the node that ran the job exits;
     - after a template rename;
     - for template names with special characters;
     - from a node that doesn't share the staging directory.
   - New `TestJobExecutorFactory`.
   - Ran:
     - `./gradlew :core:test :server:test -PskipITs`
     - `JobIT` (embedded mode)
     - `:core:spotlessCheck`, `:core:javadoc`, `:docs:build`
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
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]

Reply via email to