Copilot commented on code in PR #14120:
URL: https://github.com/apache/gravitino/pull/14120#discussion_r4344002122
##########
core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java:
##########
@@ -1682,14 +1685,23 @@ public void
testForceDropManagedCatalogPropagatesSchemaConflict() throws Excepti
// A managed-storage catalog drops each schema through its schema
operations.
Mockito.doReturn(CapabilityResult.SUPPORTED).when(capability).managedStorage(any());
Mockito.doReturn(catalog).when(catalogWrapper).catalog();
- OptimisticLockException conflict = new OptimisticLockException("lock wait
timeout");
- Mockito.doThrow(conflict).when(catalogWrapper).doWithSchemaOps(any());
+ RuntimeException failure =
+ conflict
+ ? new OptimisticLockException("lock wait timeout")
+ : new RuntimeException("Failed to delete fileset storage", new
IOException("denied"));
+ Mockito.doThrow(failure).when(catalogWrapper).doWithSchemaOps(any());
catalogManager.getCatalogCache().put(ident, catalogWrapper);
- Assertions.assertSame(
- conflict,
+ RuntimeException thrown =
Assertions.assertThrows(
- OptimisticLockException.class, () ->
catalogManager.dropCatalog(ident, true)));
+ RuntimeException.class, () -> catalogManager.dropCatalog(ident,
true));
+ if (conflict) {
+ Assertions.assertSame(failure, thrown);
+ }
+ Assertions.assertTrue(entityStore.exists(ident, EntityType.CATALOG));
+ Assertions.assertTrue(
+ entityStore.exists(
+ NameIdentifier.of("metalake", ident.name(), "schema_occ"),
EntityType.SCHEMA));
Review Comment:
For the `conflict == false` branch, the test doesn’t assert anything about
the thrown exception (message/cause/wrapping). That makes it easy for future
changes to accidentally swallow or miswrap the failure while still passing.
Consider asserting the expected error shape for the non-conflict case too
(e.g., message contains the storage-delete failure text and/or cause is an
`IOException`, depending on the intended contract).
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -925,71 +894,24 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
Optional.ofNullable(schemaEntity.properties()).orElse(Collections.emptyMap());
Map<String, Path> schemaPaths = getAndCheckSchemaPaths(ident.name(),
properties);
- boolean dropped = super.dropSchema(ident, cascade);
- if (disableFSOps) {
- // No FS cleanup; still remove fileset write-through secrets when
cascade deleted them.
- // Schema secrets are cleaned by SchemaOperationDispatcher.
- if (dropped && cascade) {
- for (FilesetEntity fileset : filesets) {
- secretManager.deleteSecretsFromProperties(fileset.properties());
- }
+ // Catalog force-drop reaches this method without
SchemaOperationDispatcher's fileset
+ // cleanup. Drop each fileset before its schema so a storage failure
preserves the failed
+ // fileset and its parent for retry. Stop at the first failure; earlier
drops stay committed.
+ if (cascade) {
+ for (FilesetEntity fileset : filesets) {
+ dropFilesetAndGet(fileset.nameIdentifier())
+ .ifPresent(
+ deleted ->
secretManager.deleteSecretsFromProperties(deleted.properties()));
}
- return dropped;
}
+ boolean dropped = super.dropSchema(ident, cascade);
// If the schema entity is failed to be deleted, we should not delete
the storage location
// and return false immediately.
- if (!dropped) {
- return false;
+ if (!dropped || disableFSOps) {
+ return dropped;
}
Review Comment:
`super.dropSchema(ident, cascade)` can re-introduce the original “cascade
deletes metadata without fileset cleanup” path if any fileset remains at that
point (e.g., due to an unexpected `Optional.empty()` from `dropFilesetAndGet`).
A safer pattern is: when `cascade == true`, manually drop filesets as you do
now, then call `super.dropSchema(ident, false)` so schema deletion will
fail/throw if anything is still present, rather than silently cascading through
the base implementation.
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -867,6 +869,139 @@ public void testDropSchemaWithFSOpsDisabled() throws
IOException {
}
}
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testDropSchemaStopsOnStorageFailureAndCanRetry(boolean throwException)
throws IOException {
+ String schemaName = "schema_cascade_failure_" + generateTestId();
+ String catalogPath = TEST_ROOT_PATH + "/" + schemaName;
+ createSchema(schemaName, "comment", catalogPath, null);
+ NameIdentifier schemaIdent = NameIdentifier.of("m1", "c1", schemaName);
+ Namespace filesetNs = Namespace.of("m1", "c1", schemaName);
+ List<FilesetEntity> filesets = new ArrayList<>();
+ List<SecretUrn> secrets = new ArrayList<>();
+ for (String name : List.of("first", "failed", "last")) {
+ SecretUrn urn =
+ SecretUrn.buildWriteThrough(
+ "memory",
+ Map.of(
+ SecretConstants.ATTR_ENTITY_TYPE, "fileset",
+ SecretConstants.ATTR_ENTITY_ID,
String.valueOf(generateTestId()),
+ SecretConstants.ATTR_PROPERTY_KEY, "test-secret"));
+ secretManager.writeSecrets(List.of(new SecretMaterial(urn,
"test-value")));
+ secrets.add(urn);
+ createFileset(
+ name,
+ schemaName,
+ "comment",
+ Fileset.Type.MANAGED,
+ catalogPath,
+ null,
+ Map.of("test-secret", urn.toString()));
+ filesets.add(
+ store.get(
+ NameIdentifier.of(filesetNs, name), Entity.EntityType.FILESET,
FilesetEntity.class));
+ }
+
+ EntityStore orderedStore = Mockito.spy(store);
+ doReturn(filesets)
+ .when(orderedStore)
+ .list(filesetNs, FilesetEntity.class, Entity.EntityType.FILESET);
+ Path failedPath = new Path(filesets.get(1).storageLocation());
+ FileSystem failingFileSystem = Mockito.mock(FileSystem.class);
+ when(failingFileSystem.exists(failedPath)).thenReturn(true);
+ if (throwException) {
+ when(failingFileSystem.delete(failedPath, true))
+ .thenThrow(new IOException("permission denied"));
+ } else {
+ when(failingFileSystem.delete(failedPath, true)).thenReturn(false);
+ }
+
+ try (FilesetCatalogOperations ops =
+ Mockito.spy(new FilesetCatalogOperations(orderedStore,
secretManager))) {
+ ops.initialize(
+ ImmutableMap.of(LOCATION, catalogPath),
+ randomCatalogInfo("m1", "c1"),
+ FILESET_PROPERTIES_METADATA);
+ doReturn(failingFileSystem)
+ .when(ops)
+ .getFileSystemWithCache(Mockito.eq(failedPath), any(Map.class));
+
+ Assertions.assertThrows(RuntimeException.class, () ->
ops.dropSchema(schemaIdent, true));
+ Assertions.assertTrue(store.exists(schemaIdent,
Entity.EntityType.SCHEMA));
+ for (int i = 0; i < filesets.size(); i++) {
+ FilesetEntity fileset = filesets.get(i);
+ Path path = new Path(fileset.storageLocation());
+ Assertions.assertEquals(
+ i != 0, store.exists(fileset.nameIdentifier(),
Entity.EntityType.FILESET));
+ Assertions.assertEquals(i != 0, path.getFileSystem(new
Configuration()).exists(path));
Review Comment:
Constructing a fresh Hadoop `Configuration` per assertion can make unit
tests more sensitive to environment-provided defaults (and it’s unnecessary
overhead inside a loop). Prefer reusing a single `Configuration` instance for
the test, or (even better) using the same configuration path/FS resolution
strategy as the operations under test to reduce flakiness.
--
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]
