LuciferYang commented on code in PR #13436:
URL: https://github.com/apache/gravitino/pull/13436#discussion_r4092955711
##########
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java:
##########
@@ -401,22 +401,35 @@ public boolean dropMetalake(NameIdentifier ident, boolean
force)
*
* <p>Callers typically {@code disableMetalake} before force-drop. {@link
* CatalogManager#dropCatalog} requires catalog {@code
metalake-in-use=true}, so a disabled
- * metalake is briefly re-enabled for child cleanup. The metalake entity is
deleted immediately
- * afterward, so the temporary enable is not restored.
+ * metalake is briefly re-enabled for child cleanup. On success the metalake
entity is deleted
+ * immediately afterward, so the temporary enable is not restored; if the
cleanup fails, the
+ * metalake is re-disabled (best effort) so a user-disabled metalake does
not stay enabled.
*/
private void dropCatalogsUnderMetalake(NameIdentifier metalakeIdent) {
if (catalogManager == null) {
return;
}
try {
- if (!metalakeInUse(store, metalakeIdent)) {
- enableMetalake(metalakeIdent);
- }
- List<CatalogEntity> catalogs =
- store.list(Namespace.of(metalakeIdent.name()), CatalogEntity.class,
EntityType.CATALOG);
- for (CatalogEntity catalog : catalogs) {
- catalogManager.dropCatalog(
- NameIdentifier.of(metalakeIdent.name(), catalog.name()), true /*
force */);
+ boolean wasDisabled = !metalakeInUse(store, metalakeIdent);
+ try {
+ if (wasDisabled) {
+ enableMetalake(metalakeIdent);
+ }
+ List<CatalogEntity> catalogs =
+ store.list(Namespace.of(metalakeIdent.name()),
CatalogEntity.class, EntityType.CATALOG);
+ for (CatalogEntity catalog : catalogs) {
+ catalogManager.dropCatalog(
+ NameIdentifier.of(metalakeIdent.name(), catalog.name()), true /*
force */);
+ }
+ } catch (NoSuchMetalakeException e) {
+ // Metalake is already gone; dropMetalake will return false. Nothing
to restore.
+ throw e;
+ } catch (IOException e) {
+ restoreDisabledState(metalakeIdent, wasDisabled);
+ throw e;
+ } catch (RuntimeException e) {
+ restoreDisabledState(metalakeIdent, wasDisabled);
+ throw e;
Review Comment:
Done. Consolidated the `IOException` and `RuntimeException` handlers into a
single `catch (IOException | RuntimeException e)` that restores and rethrows.
`NoSuchMetalakeException` (a `RuntimeException` subtype) is still caught first,
so it keeps its no-restore path.
##########
core/src/test/java/org/apache/gravitino/metalake/TestMetalakeManager.java:
##########
@@ -352,6 +352,48 @@ public void
testForceDropMetalakeAfterDisableDropsLeftoverCatalogs() throws Exce
store.close();
}
+ @Test
+ public void testFailedForceDropKeepsDisabledMetalakeDisabled() throws
Exception {
+ CatalogManager catalogManager = Mockito.mock(CatalogManager.class);
+ Object originalEnvCatalogManager =
+ FieldUtils.readField(GravitinoEnv.getInstance(), "catalogManager",
true);
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "catalogManager",
catalogManager, true);
Review Comment:
This follows the established pattern in this suite: `TestMetalakeManager`
and the other core manager tests already inject collaborators into the
`GravitinoEnv` singleton via reflection, and the core unit tests are not run in
parallel within a class. Introducing a non-global injection seam just for this
test would diverge from the surrounding tests; a broader move off the singleton
would be better as its own cleanup.
--
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]
