yuqi1129 commented on code in PR #13436:
URL: https://github.com/apache/gravitino/pull/13436#discussion_r4094486934
##########
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:
This `wasDisabled` value is read before `enableMetalake` acquires its tree
lock and rechecks the state. A concurrent user can enable the metalake between
these two reads: our `enableMetalake` then does nothing, but we still return
`true`. If catalog cleanup or the later metalake delete fails,
`restoreDisabledState` disables the metalake and silently overwrites that
user's enable. Could we track whether this operation actually changed the
state, and restore only if the same metalake is still at the version written by
that temporary enable? A test with a controlled concurrent enable between the
initial read and `enableMetalake` would catch this.
--
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]
