Copilot commented on code in PR #10488:
URL: https://github.com/apache/gravitino/pull/10488#discussion_r2972537005
##########
core/src/main/java/org/apache/gravitino/listener/api/event/OperationType.java:
##########
@@ -69,6 +69,8 @@ public enum OperationType {
ALTER_CATALOG,
LOAD_CATALOG,
LIST_CATALOG,
+ ENABLE_CATALOG,
+ DISABLE_CATALOG,
Review Comment:
New OperationType values (ENABLE/DISABLE for catalog/metalake) aren’t
recognized by the audit-compatibility layers, so deprecated audit fields like
AuditLog.Operation.fromEvent(..) / audit.v2.CompatibilityUtils will report
UNKNOWN_OPERATION for these events. Consider extending AuditLog.Operation (and
the corresponding mappings) to include enable/disable so audit logs remain
accurate for consumers still using the deprecated operation().
##########
core/src/test/java/org/apache/gravitino/listener/api/event/TestCatalogEvent.java:
##########
@@ -254,6 +288,36 @@ void testDropCatalogFailureEvent() {
Assertions.assertEquals(OperationStatus.FAILURE, event.operationStatus());
}
+ @Test
+ void testEnableCatalogFailureEvent() {
+ NameIdentifier identifier = NameIdentifier.of("metalake", catalog.name());
+ Assertions.assertThrowsExactly(
+ GravitinoRuntimeException.class, () ->
failureDispatcher.enableCatalog(identifier));
+ Event event = dummyEventListener.popPostEvent();
+ Assertions.assertEquals(identifier, event.identifier());
+ Assertions.assertEquals(EnableCatalogFailureEvent.class, event.getClass());
+ Assertions.assertEquals(
+ GravitinoRuntimeException.class,
+ ((EnableCatalogFailureEvent) event).exception().getClass());
+ Assertions.assertEquals(OperationType.ENABLE_CATALOG,
event.operationType());
+ Assertions.assertEquals(OperationStatus.FAILURE, event.operationStatus());
+ }
Review Comment:
Same test isolation concern as other failure-event tests: this test only
pops the failure post-event, leaving the pre-event in DummyEventListener and
potentially affecting later tests when the class instance is shared. Clearing
the listener per test or asserting/popping the pre-event would avoid state
leakage.
##########
core/src/main/java/org/apache/gravitino/listener/api/event/OperationType.java:
##########
@@ -92,6 +94,8 @@ public enum OperationType {
LIST_METALAKE,
DROP_METALAKE,
LOAD_METALAKE,
+ ENABLE_METALAKE,
+ DISABLE_METALAKE,
Review Comment:
Same as catalog enable/disable: these new metalake OperationType values will
currently map to UNKNOWN_OPERATION in audit formatting/migration code paths
that still rely on AuditLog.Operation / CompatibilityUtils mappings. If
enable/disable should appear in audit logs, add corresponding audit Operation
values and mapping entries.
##########
core/src/test/java/org/apache/gravitino/listener/api/event/TestMetalakeEvent.java:
##########
@@ -227,6 +261,36 @@ void testDropMetalakeFailureEvent() {
Assertions.assertEquals(OperationStatus.FAILURE, event.operationStatus());
}
+ @Test
+ void testEnableMetalakeFailureEvent() {
+ NameIdentifier identifier = NameIdentifier.of(metalake.name());
+ Assertions.assertThrowsExactly(
+ GravitinoRuntimeException.class, () ->
failureDispatcher.enableMetalake(identifier));
+ Event event = dummyEventListener.popPostEvent();
+ Assertions.assertEquals(identifier, event.identifier());
+ Assertions.assertEquals(EnableMetalakeFailureEvent.class,
event.getClass());
+ Assertions.assertEquals(
+ GravitinoRuntimeException.class,
+ ((EnableMetalakeFailureEvent) event).exception().getClass());
+ Assertions.assertEquals(OperationType.ENABLE_METALAKE,
event.operationType());
+ Assertions.assertEquals(OperationStatus.FAILURE, event.operationStatus());
+ }
Review Comment:
These failure-event tests leave the corresponding PreEvent queued in
DummyEventListener (they only pop the post/failure event). Because the test
class is PER_CLASS and the listener is shared across tests, this creates
cross-test state leakage and makes ordering matter. Consider clearing the
listener in `@BeforeEach` (dummyEventListener.clear()) or also
popping/asserting the expected PreEvent for failure paths.
--
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]
