yuqi1129 commented on PR #13147: URL: https://github.com/apache/gravitino/pull/13147#issuecomment-5663680972
Reviewed commit `6f9ab47a13`. The ownership filtering, deferred cross-node cancellation, and thread-pool fix make sense. My main concern is the new active-job expiry policy. **Blocking: an unchanged status does not mean the owner has exited.** [`isStaleActiveJob`](https://github.com/apache/gravitino/blob/6f9ab47a13a7ca3992ce5b1d4609b8642d780684/core/src/main/java/org/apache/gravitino/job/JobManager.java#L1223) uses the audit timestamp, but [`pullAndUpdateOwnedJobStatus`](https://github.com/apache/gravitino/blob/6f9ab47a13a7ca3992ce5b1d4609b8642d780684/core/src/main/java/org/apache/gravitino/job/JobManager.java#L1115) only updates metadata when the status changes. A healthy owner can keep polling a healthy `STARTED` job successfully for the entire retention period, and cleanup will still mark it `FAILED`. This has consequences beyond an inaccurate status: - The process is not stopped, but subsequent cancellation requests return immediately because the stored job is terminal. - An expired queued job remains in the executor queue and may start later despite already being reported as failed. - Later success cannot correct the terminal state. This policy also applies to external executors and single-node deployments. Documenting the limitation does not resolve the correctness issue. `stagingDirKeepTimeInMs` should remain a retention setting, rather than becoming an implicit maximum duration for an unchanged job status. For a small fix, I suggest keeping ownership/cancellation/concurrency changes and deferring automatic orphan expiry. If orphan expiry is required here, it should use explicit owner liveness (heartbeat/lease) and a separate timeout. Lease expiry also needs a defined policy for an old owner that resumes; it is not proof that the process has stopped. **Tests:** please add a case where the owner repeatedly polls a running job whose status remains `STARTED`, while another node runs cleanup after the retention threshold. The job should remain tracked and cancellable. The current expiry tests supply recent audit timestamps directly, so they do not catch the missing liveness refresh. The two-manager test also shares one store instance, global lock manager, and staging directory and invokes the operations sequentially; it does not establish behavior under independent-node concurrency. **Smaller suggestion:** I would use a full UUID for the executor identity. The probability of a 32-bit collision is low for a few live nodes, but a collision recreates the original cross-node status corruption. The execution-id column has room (`varchar(256)`), so this seems a low-cost reliability improvement. The latest-state/CAS handling and terminal-state guards otherwise look reasonable. This review is based on source and test inspection; I did not rerun the Java tests locally. -- 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]
