LuciferYang commented on code in PR #13436:
URL: https://github.com/apache/gravitino/pull/13436#discussion_r4093382828
##########
core/src/main/java/org/apache/gravitino/metalake/MetalakeManager.java:
##########
@@ -401,22 +401,32 @@ 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) {
Review Comment:
Good catch, you're right. My first fix only restored on a phase-1 (catalog
cleanup) failure, so your step-5 case (cleanup succeeds, then `store.delete`
fails) still left the metalake enabled with `wasDisabled` lost.
Fixed: `dropCatalogsUnderMetalake` now returns whether it temporarily
enabled the metalake, and `dropMetalake` restores the disabled state if the
metalake delete then fails (the metalake still exists at that point, so
`disableMetalake` succeeds). Added
`testForceDropRestoresDisabledMetalakeWhenDeleteFails`, which spies the store
to throw on the metalake delete after a successful (no-catalog) cleanup and
asserts the metalake stays disabled; it fails against the pre-fix code.
--
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]
