justinmclean opened a new issue, #10217:
URL: https://github.com/apache/gravitino/issues/10217
### What would you like to be improved?
CatalogHookDispatcher#createCatalog performs post-create hooks (setOwner,
future grant) inside a try/catch. On post-hook failure, it attempts rollback
via dispatcher.dropCatalog(ident, true), then rethrows the original exception.
If rollback also throws, the rollback exception replaces the original post-hook
exception. This masks root cause, making debugging and error attribution
incorrect.
### How should we improve?
Preserve the original post-hook exception as primary. In the catch, wrap
dropCatalog in its own try/catch; if rollback fails, attach rollback exception
as suppressed to the original (e.addSuppressed(rollbackEx)), then rethrow the
original e. Optionally log rollback failure. This keeps root cause intact while
still surfacing rollback failure details.
Here's a unit test to help:
```
public class TestCatalogHookDispatcher {
@Test
public void testCreateCatalogRollbackExceptionMasksPostHookException()
throws Exception {
GravitinoEnv gravitinoEnv = GravitinoEnv.getInstance();
Object originalOwnerDispatcher = FieldUtils.readField(gravitinoEnv,
"ownerDispatcher", true);
Object originalFutureGrantManager =
FieldUtils.readField(gravitinoEnv, "futureGrantManager", true);
CatalogDispatcher dispatcher = Mockito.mock(CatalogDispatcher.class);
Catalog catalog = Mockito.mock(Catalog.class);
NameIdentifier ident = NameIdentifier.of("metalake", "catalog");
RuntimeException postHookException = new RuntimeException("post-hook
failed");
RuntimeException rollbackException = new RuntimeException("rollback
failed");
OwnerDispatcher ownerDispatcher = Mockito.mock(OwnerDispatcher.class);
Mockito.doThrow(postHookException)
.when(ownerDispatcher)
.setOwner(Mockito.anyString(), Mockito.any(), Mockito.anyString(),
Mockito.any());
Mockito.when(
dispatcher.createCatalog(
Mockito.eq(ident),
Mockito.eq(Catalog.Type.RELATIONAL),
Mockito.eq("provider"),
Mockito.eq("comment"),
Mockito.anyMap()))
.thenReturn(catalog);
Mockito.doThrow(rollbackException).when(dispatcher).dropCatalog(ident,
true);
FieldUtils.writeField(gravitinoEnv, "ownerDispatcher", ownerDispatcher,
true);
FieldUtils.writeField(gravitinoEnv, "futureGrantManager", null, true);
try {
CatalogHookDispatcher hookDispatcher = new
CatalogHookDispatcher(dispatcher);
RuntimeException thrown =
assertThrowsExactly(
RuntimeException.class,
() ->
hookDispatcher.createCatalog(
ident,
Catalog.Type.RELATIONAL,
"provider",
"comment",
Collections.emptyMap()));
// Expected behavior: preserve original post-hook failure even if
rollback fails.
assertSame(postHookException, thrown);
Mockito.verify(dispatcher).dropCatalog(ident, true);
} finally {
FieldUtils.writeField(gravitinoEnv, "ownerDispatcher",
originalOwnerDispatcher, true);
FieldUtils.writeField(gravitinoEnv, "futureGrantManager",
originalFutureGrantManager, true);
}
}
}
```
--
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]
