swuferhong commented on code in PR #3388:
URL: https://github.com/apache/fluss/pull/3388#discussion_r3331343471
##########
fluss-server/src/main/java/org/apache/fluss/server/log/LogManager.java:
##########
@@ -561,6 +561,34 @@ public void run() {
tabletDir,
kvDeleteException.getMessage());
}
+
+ // delete empty parent directories. For partitioned
tables the
+ // parent is the partition dir and the grandparent is
+ // the table dir — both should be removed when empty.
+ // For non-partitioned tables the parent is the table
+ // dir (safe to remove) but the grandparent is the
+ // database dir — must NOT remove. Safe under parallel
+ // execution: the last job to finish finds the dir
+ // empty and removes it; deleteDirectoryQuietly
+ // tolerates races.
Review Comment:
spotless the docs.
##########
fluss-server/src/main/java/org/apache/fluss/server/log/LogManager.java:
##########
@@ -561,6 +561,34 @@ public void run() {
tabletDir,
kvDeleteException.getMessage());
}
+
+ // delete empty parent directories. For partitioned
tables the
+ // parent is the partition dir and the grandparent is
+ // the table dir — both should be removed when empty.
+ // For non-partitioned tables the parent is the table
+ // dir (safe to remove) but the grandparent is the
+ // database dir — must NOT remove. Safe under parallel
+ // execution: the last job to finish finds the dir
+ // empty and removes it; deleteDirectoryQuietly
+ // tolerates races.
+ try {
+ boolean isPartitioned =
pathAndBucket.f0.getPartitionName() != null;
+ File parentDir = tabletDir.getParentFile();
+ if (parentDir != null &&
FileUtils.isDirectoryEmpty(parentDir)) {
+ FileUtils.deleteDirectoryQuietly(parentDir);
+ if (isPartitioned) {
+ File tableDir = parentDir.getParentFile();
+ if (tableDir != null &&
FileUtils.isDirectoryEmpty(tableDir)) {
Review Comment:
ditto
##########
fluss-server/src/main/java/org/apache/fluss/server/log/LogManager.java:
##########
@@ -561,6 +561,34 @@ public void run() {
tabletDir,
kvDeleteException.getMessage());
}
+
+ // delete empty parent directories. For partitioned
tables the
+ // parent is the partition dir and the grandparent is
+ // the table dir — both should be removed when empty.
+ // For non-partitioned tables the parent is the table
+ // dir (safe to remove) but the grandparent is the
+ // database dir — must NOT remove. Safe under parallel
+ // execution: the last job to finish finds the dir
+ // empty and removes it; deleteDirectoryQuietly
+ // tolerates races.
+ try {
+ boolean isPartitioned =
pathAndBucket.f0.getPartitionName() != null;
+ File parentDir = tabletDir.getParentFile();
+ if (parentDir != null &&
FileUtils.isDirectoryEmpty(parentDir)) {
+ FileUtils.deleteDirectoryQuietly(parentDir);
Review Comment:
There's a TOCTOU race: between the `isDirectoryEmpty` check and
`deleteDirectoryQuietly`, another thread/loadLog task could create a file.
Consider using `Files.delete(path)` which fails on non-empty dirs, making this
truly safe under concurrency.
##########
fluss-server/src/main/java/org/apache/fluss/server/log/LogManager.java:
##########
Review Comment:
For the table drop case, the startup cleanup via `SchemaNotExistException`
works well.
However, for the partition drop case (table still exists, only partition
dropped while TS was offline), the orphan files won't be auto-cleaned. When the
TS restarts, loadLog() succeeds because the table schema still exists in ZK, so
the log is loaded normally as a NoneReplica. The sweepOrphanTabletDirs
mechanism requires a stopReplica(delete=true) to trigger, but the Coordinator
won't re-send it — by the time the TS reconnects, the Coordinator has already
marked the deletion as "successful" (after retry exhaustion) and removed all
tracking metadata for that partition's replicas.
I think you need consider adding a startupcheck for partition existence
(similar to the schema check) to cover this gap.
--
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]
