mchades commented on code in PR #14120:
URL: https://github.com/apache/gravitino/pull/14120#discussion_r4347025647
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -740,38 +741,7 @@ public Fileset alterFileset(NameIdentifier ident,
FilesetChange... changes)
@Override
public boolean dropFileset(NameIdentifier ident) {
- try {
- // The relational store runs this cleanup after the metadata CAS wins
but before committing
- // its transaction. The callback therefore sees the exact deleted
snapshot, and an I/O
- // failure can still restore the metadata so the caller may fix
permissions and retry.
- //
- // The price is that the recursive storage delete runs inside that
transaction, holding the
- // fileset rows and a pooled connection for as long as the filesystem
takes. Dropping a
- // fileset with a very large tree is therefore a slow write for that
fileset, and enough
- // concurrent drops can hold up the connection pool.
- Optional<FilesetEntity> deletedFileset =
- store.deleteAndGet(
- ident,
- Entity.EntityType.FILESET,
- FilesetEntity.class,
- filesetEntity -> {
- if (!disableFSOps && filesetEntity.filesetType() ==
Fileset.Type.MANAGED) {
- try {
- deleteManagedFilesetStorage(ident, filesetEntity);
- } catch (IOException ioe) {
- throw new UncheckedIOException(ioe);
- }
- }
- });
- return deletedFileset.isPresent();
- } catch (NoSuchEntityException ne) {
- LOG.warn("Fileset {} does not exist", ident);
- return false;
- } catch (UncheckedIOException uioe) {
- throw ExceptionMessages.wrap("Failed to delete fileset " + ident,
uioe.getCause());
- } catch (IOException ioe) {
- throw ExceptionMessages.wrap("Failed to delete fileset " + ident, ioe);
- }
+ return dropFilesetAndGet(ident).isPresent();
Review Comment:
The secret snapshot race in the public deletion path predates this PR. I'm
keeping that fix separate from #14119's force-drop change.
##########
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) {
+ dropFilesetAndGet(fileset.nameIdentifier())
+ .ifPresent(
+ deleted ->
secretManager.deleteSecretsFromProperties(deleted.properties()));
+ }
+
+ // Functions can also live in fileset catalogs. Drop their metadata
explicitly before
+ // the non-cascading schema delete so it still protects any
unprocessed filesets.
+ for (FunctionEntity function :
+ store.list(filesetNs, FunctionEntity.class,
Entity.EntityType.FUNCTION)) {
Review Comment:
Function cleanup now runs in the schema metadata transaction.
Missing-function failures carry function-specific context; other metadata
errors retain their exception types instead of being reported as directory
cleanup failures or a missing schema.
--
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]
