jerryshao commented on PR #13147: URL: https://github.com/apache/gravitino/pull/13147#issuecomment-5664129660
@yuqi1129 Thanks for the detailed review. This PR is intended as a temporary fix scoped to the local job executor, which keeps job state only in the memory of the server that runs the job. With an external job executor such as Airflow, any server can query and cancel a job, so these problems don't apply there. **Expiry policy.** You're right that it shouldn't affect other executors. In 8206d1d58 the expiry only applies to job executors whose job state is node local (`isJobStateNodeLocal()`), and jobs of other executors are never expired. For the local executor, a job that is still active after `gravitino.job.stagingDirKeepTimeInMs` (7 days by default) is not a normal job, so we accept treating it as left behind, as a trade-off for this temporary fix. The limitation is documented in `manage-jobs-in-gravitino.md` and the PR description. **Test.** The case you describe, a healthy owner polling a job that stays `STARTED` beyond the keep time, is expired by design with this approach, so I didn't add a test expecting it to stay tracked. A lease refreshed by the owner, with a separate timeout, would be the proper long-term solution. Agreed that the multi-node test shares one JVM, lock manager and store instance, so it covers the ownership and cancellation flow rather than true concurrency between independent servers. **Executor id.** The id only needs to be unique among the executor instances that are alive at the same time, typically a handful of servers, where the chance of a 32-bit collision is about n²/2³³ (~1e-8 for 10 instances). A collision with an instance that has already exited is harmless. So I'd keep the shorter id, as discussed in the review thread. -- 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]
