Repository navigation
[#12566] feat(core): Dispatch events for catalog connection tests - #12585
Conversation
6af707f to
50eb408
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (4)
What changed in this PR
Adds event support for catalog connection tests through the listener event bus.
Changes:
- Adds connection-test operation and event types.
- Dispatches pre/success/failure events for two connection-test overloads.
- Adds success and failure tests.
| File | Description |
|---|---|
| core/src/test/java/org/apache/gravitino/listener/api/event/TestCatalogEvent.java | Updated as part of this pull request. |
| core/src/main/java/org/apache/gravitino/listener/CatalogEventDispatcher.java | Updated as part of this pull request. |
| core/src/main/java/org/apache/gravitino/listener/api/event/TestConnectionPreEvent.java | Updated as part of this pull request. |
| core/src/main/java/org/apache/gravitino/listener/api/event/TestConnectionFailureEvent.java | Updated as part of this pull request. |
| core/src/main/java/org/apache/gravitino/listener/api/event/TestConnectionEvent.java | Updated as part of this pull request. |
| core/src/main/java/org/apache/gravitino/listener/api/event/OperationType.java | Updated as part of this pull request. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addressed Copilot review on the latest head:
Ready for re-review. |
|
Resolved 4 review threads that are addressed on current head:
|
|
could you please resolve the conflicts? |
mchades
changed the title
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open findings
4 resolved since last review
🧠 Review effort: Lite
Code Coverage Report
Files
|
- OperationType: add TEST_CONNECTION_CATALOG - add TestConnectionPreEvent/Event/FailureEvent without properties payload - CatalogEventDispatcher: dispatch pre/success/failure for testConnection - TestCatalogEvent: success and failure coverage for testConnection events Fixes apache#12566 Signed-off-by: Vedant Madane <6527493+VedantMadane@users.noreply.github.com>
Signed-off-by: Vedant Madane <6527493+VedantMadane@users.noreply.github.com>
Wrap the CatalogChange overload with pre/success/failure events, map TEST_CONNECTION_CATALOG in CompatibilityUtils/AuditLog, and cover the existing-catalog overload in tests. Signed-off-by: Vedant Madane <6527493+VedantMadane@users.noreply.github.com>
Signed-off-by: Vedant Madane <6527493+VedantMadane@users.noreply.github.com>
5d97cc1 to
e9e6c26
Compare
|
Rebased On the outdated Copilot note about pre/post event ordering in |
jerryshao
left a comment
jerryshao
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The event model, audit mapping, username capture, and exception propagation look consistent with the existing catalog events, and sensitive properties stay out of the built-in audit formatters. One small issue inline: a pre-listener veto currently surfaces as HTTP 500 instead of 403.
| ConnectionTestAction action) | ||
| throws Exception { | ||
| String user = PrincipalUtils.getCurrentUserName(); | ||
| eventBus.dispatchEvent(new TestConnectionPreEvent(user, ident, catalogInfo, changes)); |
There was a problem hiding this comment.
Pre-event veto works in CatalogEventDispatcher, but both test-connection REST methods (CatalogOperations.java:226 and :273) pass the thrown ForbiddenException to handleTestConnectionException (ExceptionHandlers.java:245), which has no forbidden branch and returns HTTP 500 via Utils.internalError. Other catalog handlers map this to Utils.forbidden (403). Please handle ForbiddenException there and add a REST test for both endpoints so a synchronous pre-listener veto stays a 403 and does not run the connection test.
There was a problem hiding this comment.
Mapped ForbiddenException to HTTP 403 in the shared handler. Added REST tests for both endpoints, covering stored configuration, proposed changes, and empty changes. They verify that a synchronous pre-listener veto never reaches the delegate and preserves the existing stack-trace behavior.
…pshots (#14276) ### What changes were proposed in this pull request? Validate Fork CI caller jobs against `required_ci.needs` of the upstream commit the run tested: the base-branch commit merged into the head for fork runs (already verified to be on the base branch), or the dispatch commit for trusted runs. A suite added later does not fail an earlier run, but a run that tested a base with the suite must have run it; a missing suite reports "merge the latest base branch and push". After a full rerun, the newest Required CI attempt selects the tested base. Share workflow contents reads with migration detection, retry transient reads on the next polling pass, and retain the existing execution, context and trust checks. Also require trusted verification when changed CI definitions report only a fail-fast watcher failure. ### Why are the changes needed? The publisher currently applies the latest hard-coded suite list to historical runs. Adding TMS on main (#14222) therefore turned successful Fork CI runs into `Fork CI needs attention` errors, even though their workflows did not contain TMS. Re-running the same old workflow cannot satisfy the newer list. On 2026-10-27 this affected 13 open PRs: #14241, #14233, #14214, #14210, #14197, #14194, #13950, #13929 (a trusted run dispatched before TMS), #13914, #13904, #13880, #13586 and #12585. Related to #14197. After merge, these statuses do not all recover at once: a completed status whose run is unchanged is treated as settled while the PR was updated in the last six hours. A committer should dispatch **Update Fork CI Status** with each PR number above; it only calls the GitHub API. ### Does this PR introduce _any_ user-facing change? Historical successful Fork CI runs keep their correct status when required callers change on main. No application API or configuration changes. ### How was this patch tested? - `node --test .github/scripts/tests/*.test.cjs`: 108 tests passed. - Read-only replay of `mirror()` against the live GitHub API (no status writes) for all 13 affected PRs: each returns `All required suites passed`, including the trusted run of #13929. - Regression tests cover added/removed callers, ordinary versus trusted definition snapshots, a newer tested base with and without a later suite, a full rerun against a newer base, a trusted run missing a suite, malformed workflow contents, API failures, shared reads, recovery from transient base and trusted lookup failures across polling passes, a missing `eligibility` caller, contents GitHub does not return inline, and the changed-definition watcher-only path. - The six initial regression tests fail against the original implementation. Mutation checks: the latest hard-coded suite list fails 9 tests, the PR base snapshot instead of the tested base fails 2, the PR base for trusted runs fails 4, reporting a changed-definition watcher-only failure as an error fails 1, and dropping the eligibility check, the inline-content check or the base read retry fails 1 each. - Replayed PR #14197's actual run/job metadata with its historical workflow: the latest list reports missing TMS; the matching historical list reports success. - `node --check` for changed scripts and `git diff --check` passed. No live commit statuses were modified during validation.
What changes were proposed in this pull request?
Dispatch pre, success, and failure events for all three catalog connection-test overloads: pre-create, stored configuration, and proposed changes. Map
TEST_CONNECTION_CATALOGin both audit implementations.Events retain the supplied pre-create catalog information or proposed changes, and failure events retain the original exception, following existing catalog event conventions. A private helper centralizes event dispatch and captures the username once.
Why are the changes needed?
Allow listeners and auditing integrations to observe catalog connection tests with the appropriate operation context.
Fixes #12566.
Does this PR introduce any user-facing change?
Listeners receive
TestConnectionPreEvent,TestConnectionEvent, andTestConnectionFailureEvent. Audit records identify these operations asTEST_CONNECTION_CATALOG.Custom listeners can access request parameters and the original exception and are responsible for redaction before logging or forwarding them. Existing built-in audit formatting and redaction behavior is unchanged.
How was this patch tested?
./gradlew :core:spotlessApply./gradlew :core:test --tests org.apache.gravitino.listener.api.event.TestCatalogConnectionEvent --tests org.apache.gravitino.listener.api.event.TestCatalogEvent --tests org.apache.gravitino.audit.TestOperation --tests org.apache.gravitino.audit.v2.TestCompatibilityUtils -PskipITs -PskipDockerTests=true --max-workers=2 --console=plain: 56 tests passed, no failures or skips.