mchades commented on code in PR #14120:
URL: https://github.com/apache/gravitino/pull/14120#discussion_r4347027822
##########
catalogs/catalog-fileset/src/test/java/org/apache/gravitino/catalog/fileset/TestFilesetCatalogOperations.java:
##########
@@ -867,6 +876,249 @@ public void testDropSchemaWithFSOpsDisabled() throws
IOException {
}
}
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testDropSchemaWithFunctions(boolean withFileset) throws IOException {
+ String schemaName = "schema_cascade_functions_" + generateTestId();
+ String catalogPath = TEST_ROOT_PATH + "/" + schemaName;
+ createSchema(schemaName, "comment", catalogPath, null);
+ NameIdentifier schemaIdent = NameIdentifier.of("m1", "c1", schemaName);
+ NameIdentifier functionIdent = NameIdentifier.of("m1", "c1", schemaName,
"function");
+ createFunction(functionIdent);
+ if (withFileset) {
+ createFileset("fileset", schemaName, "comment", Fileset.Type.MANAGED,
catalogPath, null);
+ }
+ NameIdentifier filesetIdent = NameIdentifier.of("m1", "c1", schemaName,
"fileset");
+ Path schemaPath = new Path(catalogPath, schemaName);
+ FileSystem fs = schemaPath.getFileSystem(new Configuration());
+
+ try (FilesetCatalogOperations ops = new FilesetCatalogOperations(store,
secretManager)) {
+ ops.initialize(
+ ImmutableMap.of(LOCATION, catalogPath),
+ randomCatalogInfo("m1", "c1"),
+ FILESET_PROPERTIES_METADATA);
+ Assertions.assertThrows(
+ NonEmptySchemaException.class, () -> ops.dropSchema(schemaIdent,
false));
+ Assertions.assertTrue(store.exists(schemaIdent,
Entity.EntityType.SCHEMA));
+ Assertions.assertTrue(store.exists(functionIdent,
Entity.EntityType.FUNCTION));
+ Assertions.assertEquals(withFileset, store.exists(filesetIdent,
Entity.EntityType.FILESET));
+ Assertions.assertTrue(fs.exists(schemaPath));
+
+ Assertions.assertTrue(ops.dropSchema(schemaIdent, true));
+ Assertions.assertFalse(store.exists(schemaIdent,
Entity.EntityType.SCHEMA));
+ Assertions.assertFalse(store.exists(functionIdent,
Entity.EntityType.FUNCTION));
+ Assertions.assertFalse(store.exists(filesetIdent,
Entity.EntityType.FILESET));
+ Assertions.assertFalse(fs.exists(schemaPath));
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testDropSchemaStopsOnStorageFailureAndCanRetry(boolean throwException)
throws IOException {
Review Comment:
Added cases where the second location deletion returns false or throws
IOException. Both verify metadata and secret retention and successful retry
with the first location already absent.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -925,71 +895,32 @@ 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) {
Review Comment:
Yes. This keeps metadata recoverable on cleanup failure and stops the loop
at the first failure. Each fileset has its own transaction, but large trees
hold a connection longer. Parallel or asynchronous cleanup should be considered
separately.
--
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]
