yuqi1129 opened a new pull request, #13047: URL: https://github.com/apache/gravitino/pull/13047
### What changes were proposed in this pull request? - Add `DeleteJobEvent`, so a listener can learn that a job is gone. Job templates already had `Register`, `Alter` and `Delete` events, while jobs had only `Run`, `Cancel`, `Get` and `List`. - Give `JobManager` a listener, injected through the constructor, that it calls once per job its staging directory cleanup deletes. `GravitinoEnv` passes one that dispatches `DeleteJobEvent` on the event bus. - Register `DELETE_JOB` in `AuditLog.Operation` and in the v2 `CompatibilityUtils` mapping, without which the audit log records it as `UNKNOWN_OPERATION`. Constructor injection keeps `JobManager` free of a dependency on the event bus and keeps the field final. A listener that throws is logged and does not stop the cleanup from reaching the remaining jobs, which are already deleted from the store by that point. ### Why are the changes needed? `JobManager.cleanUpStagingDirs()` is the only path that deletes a job. It runs on a scheduler inside `JobManager`, which sits at the bottom of the chain assembled in `GravitinoEnv`: ``` JobHookDispatcher -> JobEventDispatcher(eventBus) -> JobTemplateValidationDispatcher -> JobManager ``` The deletion therefore never passes through `JobEventDispatcher`, and no listener observes it. A listener that keeps a projection of jobs serves jobs that no longer exist, and its store grows without bound, because the one path that removes a job is invisible to it. Fix: #13043 ### Does this PR introduce _any_ user-facing change? Yes, all additive: - New event `DeleteJobEvent`, with `deletedJobInfo()` and operation type `DELETE_JOB`. - New `OperationType.DELETE_JOB` and `AuditLog.Operation.DELETE_JOB`. Job deletions now appear in the audit log rather than being absent. - New `JobManager` constructor taking the deletion listener. The existing constructors keep working and default to doing nothing, so no caller has to change. One point I would like reviewers' opinion on: the cleanup runs on the server's own schedule, so no caller exists to attribute the deletion to, and `Event` requires a user. I added `DeleteJobEvent.SYSTEM_USER` for that case rather than reusing the anonymous user, which means an unauthenticated caller and would be misleading here. Happy to switch to another convention if the project has one. ### How was this patch tested? - `TestJobManager.testCleanUpStagingDirsNotifiesDeletionListener` covers that the cleanup reports each job it deletes. Written first: it failed to compile against the old API, which is what proves it exercises the new path. - `TestDeleteJobEvent` covers the event's identifier, operation type, operation status and payload. - `./gradlew :core:test :core:javadoc -PskipITs` The full `:core` suite is what caught the missing audit mapping: `TestCompatibilityUtils.testAllKnownOperationTypesMapToConcreteAuditOperation` fails for any `OperationType` that has no concrete audit operation, and the targeted job tests were all green while that was still broken. -- 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]
