Copilot commented on code in PR #13436:
URL: https://github.com/apache/gravitino/pull/13436#discussion_r4073202353
##########
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 test mutates the singleton `GravitinoEnv` via reflection, which can
create test-order and parallel-execution races if the suite runs concurrently.
If the project runs tests in parallel, prefer isolating this with a non-global
injection mechanism (e.g., constructing `MetalakeManager` without touching
`GravitinoEnv`, or using a dedicated test-only environment instance), or mark
the test/class to run non-parallel to avoid cross-test interference.
##########
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:
The same restore+rethrow logic is duplicated in the `IOException` and
`RuntimeException` handlers. Consider consolidating by catching a broader type
once (e.g., `Exception` excluding `NoSuchMetalakeException`), calling
`restoreDisabledState(...)`, then rethrowing. This reduces duplication and
makes it harder to miss restoration for future exception types thrown from
`store.list(...)` or `dropCatalog(...)`.
--
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]
