yuqi1129 commented on PR #12855: URL: https://github.com/apache/gravitino/pull/12855#issuecomment-5522912380
The lifecycle-event direction is useful, and the current happy-path payload plus v1/v2 operation mappings are mostly correct. I do not think this is ready to merge until the snapshot consistency and pre-event ordering issues in the inline comments are resolved. Recommended design: 1. Emit a pre-event containing only mutation intent so listeners can veto before any domain read or write. 2. Execute the point mutation in TagManager / PolicyTagRelService and return a domain-level PolicyTagRelationMutationResult containing previous, resulting or removed, and changed, captured by the same transaction. 3. Build the success event from that actual result. Build the failure event from the original exception without dispatching a late pre-event. 4. If previous state is mandatory in the pre-event, perform a point lookup that returns a version token and require the subsequent mutation to CAS that version. Do not hold a database or tree lock while invoking arbitrary listeners. This removes the O(N) relation scan, keeps JSON and RelationalEntity knowledge out of the event layer, makes the event describe the mutation that actually committed, and provides a reusable result for the future REST and audit layers. Before finalizing the public event API, please also choose one authoritative contract: create-only add with duplicate conflict, or idempotent PUT/upsert with selector replacement. The current API and the design document specify different behavior. Merge-gating tests should cover: - exact PRE -> SUCCESS and PRE -> FAILURE ordering, with one delegate mutation; - pre-listener veto before any business read/write; - snapshot/storage failure without exception masking, including a throwing pre listener; - create, duplicate or idempotent repeat, selector replacement if supported, remove-existing, and remove-missing; - deterministic concurrent remove/remove and remove/add interleavings, asserting that the event matches the mutation committed by that request; - point selection when a tag has multiple policy associations; - actual v1/v2 formatted audit output, including enough information to identify both sides of the relation; - an integration test using the real TagManager/storage path rather than only stateless mocks. For maintainability, consider one canonical OperationType-to-audit mapping shared by v1 and v2. The earlier missed v1 branch demonstrates the risk of maintaining two independent registries. The forwarding hook/event decorators have a similar compile-time gap because new default dispatcher methods can silently fall through to UnsupportedOperationException; a forwarding base or abstract internal methods would make future additions safer. -- 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]
