Copilot commented on code in PR #12585:
URL: https://github.com/apache/gravitino/pull/12585#discussion_r4247224430
##########
core/src/main/java/org/apache/gravitino/listener/CatalogEventDispatcher.java:
##########
@@ -218,20 +221,43 @@ 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;
+ }
Review Comment:
This try/catch + dispatch pattern (and repeated
`PrincipalUtils.getCurrentUserName()` calls) is duplicated across all three
`testConnection(...)` overloads. Consider capturing the username once in a
local variable and extracting a small private helper that accepts the
connection-test action (e.g., via a functional interface) to centralize the
pre/success/failure dispatch logic. This reduces repeated code paths and makes
future changes (e.g., payload changes, new event types) less error-prone.
##########
core/src/test/java/org/apache/gravitino/listener/api/event/TestCatalogEvent.java:
##########
@@ -357,6 +363,72 @@ void testListCatalogInfoFailureEvent() {
Assertions.assertEquals(OperationStatus.FAILURE, event.operationStatus());
}
+ @Test
+ void testTestConnectionEvent() throws Exception {
+ NameIdentifier identifier = NameIdentifier.of("metalake", catalog.name());
+ dispatcher.testConnection(
+ identifier, catalog.type(), catalog.provider(), catalog.comment(),
catalog.properties());
+ Event event = dummyEventListener.popPostEvent();
+ Assertions.assertEquals(identifier, event.identifier());
+ Assertions.assertEquals(TestConnectionEvent.class, event.getClass());
+ Assertions.assertEquals(OperationType.TEST_CONNECTION_CATALOG,
event.operationType());
+ Assertions.assertEquals(OperationStatus.SUCCESS, event.operationStatus());
+
+ PreEvent preEvent = dummyEventListener.popPreEvent();
+ Assertions.assertEquals(identifier, preEvent.identifier());
+ Assertions.assertEquals(TestConnectionPreEvent.class, preEvent.getClass());
+ Assertions.assertEquals(OperationType.TEST_CONNECTION_CATALOG,
preEvent.operationType());
+ Assertions.assertEquals(OperationStatus.UNPROCESSED,
preEvent.operationStatus());
Review Comment:
These tests validate fields but not the relative ordering of pre vs post
events (and they pop the post-event before the pre-event). To ensure
regressions in dispatch order are caught, pop/assert the pre-event first and/or
add an explicit ordering assertion (e.g., via an ordered list, timestamps, or a
single queue in the listener if available).
--
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]
