Copilot commented on code in PR #12585:
URL: https://github.com/apache/gravitino/pull/12585#discussion_r4089294398
##########
core/src/main/java/org/apache/gravitino/listener/CatalogEventDispatcher.java:
##########
@@ -218,14 +221,33 @@ public void testConnection(
String comment,
Map<String, String> properties)
throws Exception {
- // TODO(#12566): Support event dispatching for testConnection
- dispatcher.testConnection(ident, type, provider, comment, properties);
+ // Do not put properties on the event payload - they may contain
credentials (#12566).
+ eventBus.dispatchEvent(
+ new TestConnectionPreEvent(PrincipalUtils.getCurrentUserName(),
ident));
+ try {
+ dispatcher.testConnection(ident, type, provider, comment, properties);
+ eventBus.dispatchEvent(
+ new TestConnectionEvent(PrincipalUtils.getCurrentUserName(), ident));
+ } catch (Exception e) {
+ eventBus.dispatchEvent(
+ new TestConnectionFailureEvent(PrincipalUtils.getCurrentUserName(),
ident, e));
+ throw e;
+ }
}
@Override
public void testConnection(NameIdentifier ident) throws Exception {
- // TODO(#12566): Support event dispatching for testConnection
- dispatcher.testConnection(ident);
+ eventBus.dispatchEvent(
+ new TestConnectionPreEvent(PrincipalUtils.getCurrentUserName(),
ident));
+ try {
+ dispatcher.testConnection(ident);
Review Comment:
The new event lifecycle covers the pre-creation and stored-configuration
overloads, but the third `testConnection(NameIdentifier, CatalogChange...)`
overload still delegates directly. The REST existing-catalog endpoint uses that
overload whenever proposed changes are supplied, so those connection tests emit
no events. Add the same pre/success/failure wrapping there, keeping the changes
out of the event payload.
##########
core/src/main/java/org/apache/gravitino/listener/CatalogEventDispatcher.java:
##########
@@ -47,6 +47,9 @@
import org.apache.gravitino.listener.api.event.EnableCatalogEvent;
import org.apache.gravitino.listener.api.event.EnableCatalogFailureEvent;
import org.apache.gravitino.listener.api.event.EnableCatalogPreEvent;
+import org.apache.gravitino.listener.api.event.TestConnectionEvent;
+import org.apache.gravitino.listener.api.event.TestConnectionFailureEvent;
+import org.apache.gravitino.listener.api.event.TestConnectionPreEvent;
Review Comment:
The new imports are out of lexical order: `TestConnection*` appears before
the existing `ListCatalog*` and `LoadCatalog*` imports. This violates the
file's established Google Java import ordering and will be reformatted by the
project's Java formatter; move the three new imports after
`LoadCatalogPreEvent`.
##########
core/src/main/java/org/apache/gravitino/listener/api/event/OperationType.java:
##########
@@ -73,6 +73,7 @@ public enum OperationType {
LIST_CATALOG,
ENABLE_CATALOG,
DISABLE_CATALOG,
+ TEST_CONNECTION_CATALOG,
Review Comment:
Adding this enum value without updating the audit compatibility map makes
`CompatibilityUtils.toAuditLogOperation(TEST_CONNECTION_CATALOG)` return
`UNKNOWN_OPERATION`; the existing
`testAllKnownOperationTypesMapToConcreteAuditOperation` test will fail, and new
events will be downgraded in the v2-to-legacy audit path. Add a corresponding
legacy operation mapping (and event conversion handling) with this enum
addition.
##########
core/src/test/java/org/apache/gravitino/listener/api/event/TestCatalogEvent.java:
##########
@@ -410,4 +410,42 @@ private CatalogDispatcher mockExceptionCatalogDispatcher()
{
});
return dispatcher;
}
+
+ @Test
+ void testTestConnectionEvent() throws Exception {
+ NameIdentifier identifier = NameIdentifier.of("metalake", catalog.name());
+ dispatcher.testConnection(
+ identifier, catalog.type(), catalog.provider(), catalog.comment(),
catalog.properties());
Review Comment:
These tests exercise only the pre-create overload that accepts catalog
properties. The newly added existing-catalog overload at
`CatalogEventDispatcher.testConnection(NameIdentifier)` has no success or
failure coverage, so its event class/status and exception path can regress
unnoticed; add both cases (and verify the pre-event as appropriate).
--
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]
