mchades commented on code in PR #13506:
URL: https://github.com/apache/gravitino/pull/13506#discussion_r4339315895
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -950,7 +990,7 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
// the schema path.
ClassLoader cl = Thread.currentThread().getContextClassLoader();
filesets.parallelStream()
- .filter(f -> f.filesetType() == Fileset.Type.MANAGED)
+ .filter(this::deletesStorageOnDrop)
Review Comment:
[P1] Honor committed retention changes during cascade
`filesets` is read before `super.dropSchema(...)`, so this filter uses a
stale snapshot. In a multi-server deployment, another server can successfully
set `delete-data-on-drop=false` after that read but before the metadata
deletion; the cascade still sees the old value and deletes the data. I
reproduced this interleaving, and forced catalog drops also use this path.
Please evaluate retention against the version actually being deleted, as
`dropFileset` does, or abort if it changed.
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogOperations.java:
##########
@@ -578,6 +585,25 @@ public Fileset createMultipleLocationFileset(
formalizePath,
entry.getKey());
} else {
+ // A managed fileset takes its data with it on drop, so silently
adopting a location
+ // that already holds files would put data Gravitino never created
at risk.
+ if (type == Fileset.Type.MANAGED
Review Comment:
[P2] Validate every location before creating directories
This check runs in the same loop as `mkdirs`. With fresh locations
`a=/new/root/child` and `b=/new/root`, processing `a` first creates `b` as a
non-empty parent, so the request is rejected even though neither location
existed before the call. I reproduced this on the PR head; the pre-change
implementation accepts it. When a later location genuinely contains existing
data, earlier newly created directories are also left behind after rejection.
Please preflight all locations before creating any of them.
--
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]
