Copilot commented on code in PR #3388:
URL: https://github.com/apache/fluss/pull/3388#discussion_r3329996339
##########
fluss-server/src/main/java/org/apache/fluss/server/log/LogManager.java:
##########
@@ -543,9 +543,9 @@ public void run() {
FileUtils.deleteDirectoryQuietly(tabletDir);
// Also delete corresponding KV tablet directory if it
exists
+ Tuple2<PhysicalTablePath, TableBucket> pathAndBucket =
+ FlussPaths.parseTabletDir(tabletDir);
try {
Review Comment:
Moving `parseTabletDir(tabletDir)` outside the surrounding `try { ... }
catch (Exception kvDeleteException)` block changes error semantics: if parsing
throws, it now escapes the SchemaNotExistException handler and is rethrown as
`FlussRuntimeException`, whereas previously the failure was tolerated with a
`LOG.warn`. This makes startup more brittle for malformed/legacy tablet
directory names. Consider wrapping `parseTabletDir` and the subsequent KV +
parent cleanup in a single try/catch that logs and continues, since both KV
cleanup and the new parent-dir cleanup depend on `pathAndBucket`.
##########
fluss-server/src/main/java/org/apache/fluss/server/replica/ReplicaManager.java:
##########
@@ -1927,6 +1941,57 @@ private StopReplicaResultForBucket stopReplica(
return new StopReplicaResultForBucket(tb);
}
+ /**
+ * Remove on-disk tablet directories for a bucket that the in-memory
ReplicaManager does not
+ * know about. This handles the case where a stopReplica(delete=true)
arrives after the
+ * TabletServer was restarted during a delete — LogManager loaded the log
at startup but no
+ * NotifyLeaderAndIsr ever ran, so allReplicas is empty.
+ */
+ private void sweepOrphanTabletDirs(
+ TableBucket tb, Map<Long, Path> deletedTableIds, Map<Long, Path>
deletedPartitionIds) {
+ Optional<LogTablet> orphanLog = logManager.getLog(tb);
+ if (!orphanLog.isPresent()) {
+ return;
+ }
+
+ LogTablet logTablet = orphanLog.get();
+ File dataDir = logTablet.getDataDir();
+ PhysicalTablePath physicalTablePath = logTablet.getPhysicalTablePath();
+ Path tabletParentDir = logManager.getTabletParentDir(dataDir,
physicalTablePath, tb);
+
+ logManager.dropLog(tb);
+
+ boolean isKvTable = false;
+ if (kvManager.getKv(tb).isPresent()) {
+ kvManager.dropKv(tb);
+ isKvTable = true;
+ } else {
+ File kvTabletDir = FlussPaths.kvTabletDir(dataDir,
physicalTablePath, tb);
+ if (kvTabletDir.exists()) {
+ isKvTable = true;
+ try {
+ FileUtils.deleteDirectory(kvTabletDir);
+ } catch (IOException e) {
+ throw new KvStorageException(
+ String.format(
+ "Failed to delete orphan KV tablet
directory %s", kvTabletDir),
+ e);
+ }
+ }
+ }
+
+ localDiskManager.recordReplicaDelete(dataDir, isKvTable);
+
+ if (tb.getPartitionId() != null) {
+ deletedPartitionIds.put(tb.getPartitionId(), tabletParentDir);
+ deletedTableIds.put(tb.getTableId(), tabletParentDir.getParent());
+ } else {
+ deletedTableIds.put(tb.getTableId(), tabletParentDir);
+ }
Review Comment:
`sweepOrphanTabletDirs` is not atomic: if `kvManager.dropKv(tb)` or the
direct `FileUtils.deleteDirectory(kvTabletDir)` throws after
`logManager.dropLog(tb)` has already removed the log tablet, the bucket is
partially cleaned but `localDiskManager.recordReplicaDelete(...)` is never
called and the parent dir is never recorded in
`deletedPartitionIds`/`deletedTableIds`. The stopReplica response also reports
an error to the coordinator, but the log is already gone, so a coordinator
retry would re-enter `NoneReplica` and early-return at
`!orphanLog.isPresent()`, never finishing KV cleanup or accounting. Consider
performing KV cleanup before `dropLog`, or doing disk accounting / parent-dir
tracking even when KV cleanup fails (e.g. in a `finally` after the log is
dropped).
--
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]
