mchades commented on code in PR #14120:
URL: https://github.com/apache/gravitino/pull/14120#discussion_r4344467274
##########
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:
Added assertions for the schema context and preservation of the original
storage failure in the cause chain.
##########
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:
Both assertion loops now reuse one real local FileSystem, independent of the
mocked filesystem used to inject failures.
--
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]
