Copilot commented on code in PR #10665:
URL: https://github.com/apache/gravitino/pull/10665#discussion_r3045263132
##########
core/src/main/java/org/apache/gravitino/listener/AccessControlEventDispatcher.java:
##########
@@ -244,7 +244,7 @@ public boolean removeGroup(String metalake, String group)
throws NoSuchMetalakeE
return isExists;
} catch (Exception e) {
- eventBus.dispatchEvent(new RemoveGroupFailureEvent(initiator, metalake,
e, group));
+ eventBus.dispatchEventSafe(new RemoveGroupFailureEvent(initiator,
metalake, e, group));
throw e;
Review Comment:
`AccessControlEventDispatcher` now uses `dispatchEventSafe()` for many
failure events, but some failure paths in the same class still call
`eventBus.dispatchEvent(...)` (e.g., `revokeRolesFromGroup`, `createRole`,
`grantPrivilegeToRole`, `revokePrivilegesFromRole`,
`overridePrivilegesInRole`). Those remaining calls can still allow listener
exceptions to mask the original operation exception, which seems inconsistent
with the PR description and intent. Consider switching the remaining
`*FailureEvent` dispatches in this class to `dispatchEventSafe()` as well for
consistent behavior.
##########
core/src/main/java/org/apache/gravitino/listener/EventBus.java:
##########
@@ -85,6 +89,31 @@ public Optional<BaseEvent> dispatchEvent(BaseEvent
baseEvent) {
}
}
+ /**
+ * Safely dispatches an event to all registered listeners without
propagating exceptions.
+ *
+ * <p>This method wraps {@link #dispatchEvent(BaseEvent)} with exception
handling. If any
+ * exception occurs during dispatch (including listener errors or unknown
event types), it is
+ * caught and logged without being propagated to the caller. This is
particularly useful when
+ * dispatching failure events to prevent secondary exceptions from masking
the original error.
+ *
+ * @param baseEvent the event to dispatch
+ * @return an Optional containing the transformed pre-event if it implements
{@link
+ * SupportsChangingPreEvent}, otherwise {@link Optional#empty() empty}.
Returns empty if an
+ * exception occurs.
+ */
+ public Optional<BaseEvent> dispatchEventSafe(BaseEvent baseEvent) {
+ try {
+ return dispatchEvent(baseEvent);
+ } catch (Exception e) {
+ LOG.error(
+ "Failed to dispatch event: {}, ignoring exception and continuing",
+ baseEvent == null ? "null" : baseEvent.getClass().getSimpleName(),
+ e);
+ return Optional.empty();
+ }
Review Comment:
`dispatchEventSafe()` currently catches all `Exception`, which will also
swallow control-flow exceptions like `ForbiddenException` from pre-events if
this method is ever used with `PreEvent`. Consider narrowing what gets
swallowed (e.g., only `RuntimeException` from listeners, and/or rethrow
`ForbiddenException`), or otherwise constraining/documenting this method so it
can’t silently change pre-event semantics.
##########
core/src/test/java/org/apache/gravitino/listener/api/event/TestGroupEvent.java:
##########
@@ -362,6 +364,62 @@ void testRemoveGroupFailureEvent() {
Assertions.assertEquals(groupName, removeGroupFailureEvent.groupName());
}
+ @Test
+ void testRemoveGroupFailureEventDoesNotMaskOriginalException() {
+ GravitinoRuntimeException originalException =
+ new GravitinoRuntimeException("Original remove group failure");
+ GravitinoRuntimeException listenerException =
+ new GravitinoRuntimeException("Listener failed while handling failure
event");
+
+ AccessControlDispatcher exceptionDispatcher =
mock(AccessControlDispatcher.class);
+ when(exceptionDispatcher.removeGroup(METALAKE,
groupName)).thenThrow(originalException);
+
+ // Create a listener that throws when handling failure events
+ AccessControlEventDispatcher dispatcherWithThrowingListener =
+ createExceptionEventDispatcher(listenerException, exceptionDispatcher);
+
+ // The original exception should be propagated, not the listener's
exception
+ GravitinoRuntimeException thrownException =
+ Assertions.assertThrows(
+ GravitinoRuntimeException.class,
+ () -> dispatcherWithThrowingListener.removeGroup(METALAKE,
groupName));
+
+ // Verify it's the original exception, not the listener's exception
+ Assertions.assertSame(originalException, thrownException);
+ }
+
+ private static AccessControlEventDispatcher createExceptionEventDispatcher(
+ GravitinoRuntimeException listenerException, AccessControlDispatcher
exceptionDispatcher) {
+ EventListenerPlugin throwingListener =
+ new EventListenerPlugin() {
+ @Override
+ public void init(java.util.Map<String, String> properties) {}
+
+ @Override
Review Comment:
Avoid using a fully qualified type in the `init` method signature
(`java.util.Map`). Import `java.util.Map` at the top of the test and use
`Map<String, String>` for consistency with the project’s Java import style.
--
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]
