LuciferYang commented on code in PR #13436:
URL: https://github.com/apache/gravitino/pull/13436#discussion_r4096249274
##########
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java:
##########
@@ -401,30 +410,65 @@ 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. If the cleanup fails it
is re-disabled here
+ * (best effort); if the cleanup succeeds this returns whether the metalake
was temporarily
+ * enabled, so {@code dropMetalake} can re-disable it should the metalake
delete then fail. Either
+ * way a user-disabled metalake does not stay enabled after a failed force
drop.
+ *
+ * @param metalakeIdent the metalake whose child catalogs are force-dropped
+ * @return {@code true} if this temporarily enabled a user-disabled
metalake, so the caller must
+ * restore the disabled state if the subsequent metalake delete fails
*/
- private void dropCatalogsUnderMetalake(NameIdentifier metalakeIdent) {
+ private boolean dropCatalogsUnderMetalake(NameIdentifier metalakeIdent) {
if (catalogManager == null) {
- return;
+ return false;
}
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);
Review Comment:
Good catch. I made the enable decision atomic: the check-and-flip now runs
inside the metalake write lock via a new private `enableMetalakeIfDisabled`,
and the restore is keyed on a flag that is set only when this operation
actually performed the disabled→enabled flip, not on an earlier unlocked
`metalakeInUse` read. A concurrent enable can no longer make the force drop
believe it enabled the metalake and then re-disable it, and a failure partway
through the enable still restores the disabled state.
Tests: `testFailedForceDropRestoresWhenTemporaryEnableFailsMidway` covers a
temporary enable that throws while pushing the in-use status to catalogs, and
`testForceDropDeleteFailureDoesNotDisableAlreadyEnabledMetalake` covers an
already-enabled metalake whose phase-2 delete fails (it must stay enabled).
One residual I'll track as a follow-up: if a concurrent user disables and
then re-enables during the catalog-cleanup or delete window, a later failure's
restore would still undo that re-enable. Closing it fully needs optimistic
concurrency on the metalake entity (a monotonic revision to CAS on), which we
don't expose today (`getVersion()` is the schema version, not an entity
revision), so it's a storage-layer change beyond this PR.
--
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]
