nevzheng opened a new pull request, #12692: URL: https://github.com/apache/gravitino/pull/12692
## Summary An inner `TableDispatcher` layer can now attach `Map<String, String>` audit facts to the terminal table event that `TableEventDispatcher` already emits, so one operation still produces one audit record and that record can say what the dispatch chain knew about it. The change is deliberately confined to carrying facts. Dispatch semantics are untouched: events still go out through `eventBus.dispatchEvent`, and a listener exception propagates exactly as it does on `main`, whether or not extras were stashed. ## Context Gravitino's dispatch chain is layered, and the layer that knows something audit-worthy about an operation is usually not the layer that emits the event. An inner layer that applied a policy or enforced a validation had no way to get that fact into the `CreateTableEvent` the outer dispatcher publishes — it could publish a separate event, which breaks the one-operation-one-event shape audit consumers rely on, or drop the information. This fills a gap in a facility the event API already has rather than proposing a new one. `BaseEvent` declares `customInfo()` returning an empty map, marked `@since 0.8.0` (`core/src/main/java/org/apache/gravitino/listener/api/event/BaseEvent.java:131`). `AuthorizationDenialFailureEvent` and `HttpRequestFailureEvent` already override it, both cases where the reason for an outcome was the point. Both audit formatters already serialize it with redaction (`audit/JsonAuditFormatter.java:69`, `audit/v2/SimpleAuditLogV2.java:101`, `audit/AuditLogRedactor.java`). The consumption side was therefore already built end to end. What was missing is that only the code constructing an event can populate `customInfo`, and for table events that is always the outermost dispatcher. No listener or formatter changes are needed. ## Changes - **New**: `TableEvent` and `TableFailureEvent` gain a `protected` constructor accepting custom info and an override of `customInfo()` that returns it, defensively copied and normalized so `null` and empty both yield an empty map. The six concrete table events gain a matching `public` overload. - **New**: `RequestContext.setAuditExtras` and `takeAuditExtras`. A contributing layer stashes facts before it returns or throws; `TableEventDispatcher` takes them when building the terminal event. The read is read-and-clear, and `RequestContext.clear()` now removes the stash during request teardown, so a fact cannot follow a pooled thread into a later request. - **Unchanged**: every existing constructor keeps its signature and delegates with an empty map. `EventBus` is untouched. Error propagation and operation results are untouched. - **Deferred**: the other fourteen dispatchers (`Catalog`, `Schema`, `Fileset`, `Topic`, `Model`, `View`, `Function`, `Partition`, `Policy`, `Tag`, `Statistic`, `Job`, `Metalake`, `AccessControl`) have the same gap. Kept out to keep this reviewable. ## Impact and risks No behavior change for existing deployments and no configuration change. The API surface is additive only: new constructor overloads on the six table events, new protected constructors on the two base classes, and the two `RequestContext` methods. Existing constructors now produce events whose `customInfo()` is an empty map, which is what `BaseEvent` already returned for them. Audit output changes only when a layer actually stashes facts. In that case the existing `customInfo` field is populated instead of empty; the record's shape, count, and every other field are unchanged. The risk worth naming is the threading contract — see below. ## Reviewer focus **The `ThreadLocal` is the debatable part and the thing I most want a read on.** Passing the facts as an argument would be cleaner, but the contributing layer and the emitting layer are not in the same call frame: `TableEventDispatcher.createTable` calls `dispatcher.createTable(...)`, gets back a `Table`, then builds the event. Threading a value through means changing the `TableDispatcher` interface so every implementation returns or accepts an audit channel — a wide blast radius for a facility most implementations will never use. `RequestContext` already holds per-request state of exactly this shape, so this reuses it rather than adding a parallel mechanism. The cost is real: correctness depends on the contributor and the dispatcher running on the same thread. That holds for the servlet request path, but it is an invariant a future async dispatch layer could break. `TestRequestContext` pins the thread-confinement assumption explicitly so the constraint is visible and a violation fails loudly rather than silently mis-attributing a fact. Second, scope. If you would rather see the pattern applied across all fifteen dispatchers in one change than land it on tables first, say so and I will extend it. ## Verification Environment: macOS, Gradle 8.2 wrapper, Amazon Corretto JDK 17.0.19. All from the repository root. - `./gradlew :core:compileJava :core:compileTestJava` — BUILD SUCCESSFUL. - `./gradlew :core:test -PskipITs --tests '*TableEventDispatcher*' --tests '*TestTableEvent*' --tests '*TestRequestContext*' --tests '*TestOperation*'` — BUILD SUCCESSFUL. Confirmed against the JUnit XML that the tests ran rather than being filtered out: 41 tests, 0 failures, 0 errors (`TestTableEventDispatcher` 3, `TestTableEvent` 16, `TestRequestContext` 9, `TestOperation` 13; the latter three include pre-existing tests in those classes). - `./gradlew spotlessApply` then `./gradlew spotlessJavaCheck` — BUILD SUCCESSFUL, no drift. Coverage added: - `TestTableEventDispatcher` (new): stashed facts reach `CreateTableEvent.customInfo()` with exactly one post event produced; facts do not leak into a second operation on the same thread; and listener-failure propagation is identical with and without extras, which is the assertion that would catch this change starting to affect error handling. - `TestTableEvent`: success, failure, and alter/load cases driven through a real dispatcher, asserting empty `customInfo()` on the pre-existing cases so the default stays empty. - `TestRequestContext`: set/take/clear semantics, empty-and-null retraction, and thread confinement of the stash. - `TestOperation`: events built with the new constructors still classify as `CREATE_TABLE`. **Not run**: the full `./gradlew build`, integration tests, and other modules' suites. The change is confined to `core`; CI is the check for the rest. ## Related work Fix: #12691 cc @roryqi @jerryshao @lasdf for review. Nevin Sent from my 🤖 (Cursor) -- 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]
