smengcl commented on code in PR #11009:
URL: https://github.com/apache/ozone/pull/11009#discussion_r4069448758
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -824,12 +920,25 @@ public BackgroundTaskResult call() {
} else if (!isPreviousPurgeTransactionFlushed()) {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot =
snapInfo == null ? null :
- omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(),
snapInfo.getBucketName(),
- snapInfo.getName())) {
- KeyManager keyManager = snapInfo == null ?
getOzoneManager().getKeyManager()
- : omSnapshot.get().getKeyManager();
- processDeletedDirsForStore(snapInfo, keyManager, run,
pathLimitPerTask);
+ boolean snapshotProcessingLockAcquired = false;
+ try {
+ if (snapInfo != null) {
+ // Serialize snapshot setup before opening the snapshot DB.
Workers still process each snapshot in
+ // parallel, but another snapshot task cannot wait here while
retaining a task-owned DB handle.
+ snapshotProcessingLock.lockInterruptibly();
+ snapshotProcessingLockAcquired = true;
+ }
+ try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot =
snapInfo == null ? null :
+ omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(),
snapInfo.getBucketName(),
+ snapInfo.getName())) {
+ KeyManager keyManager = snapInfo == null ?
getOzoneManager().getKeyManager()
+ : omSnapshot.get().getKeyManager();
+ processDeletedDirsForStore(snapInfo, keyManager, omSnapshot,
run, pathLimitPerTask);
+ }
+ } finally {
+ if (snapshotProcessingLockAcquired) {
+ snapshotProcessingLock.unlock();
+ }
}
Review Comment:
The lock covers the whole task, and the setup wording was misleading.
Releasing it immediately after `getActiveSnapshot()` would allow another task
to retain a DB handle while waiting for the shared worker pool. The revision
replaces the lock with one BackgroundService coordinator for AOS and snapshot
tasks. The separate deletion pool retains the configured worker parallelism.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
Review Comment:
Added the method summary and documented the coordinator requirement and
current-snapshot handle ownership.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -639,7 +648,8 @@ private
OzoneManagerProtocolProtos.SetSnapshotPropertyRequest getSetSnapshotRequ
* @param keyManager KeyManager of the underlying store.
*/
@VisibleForTesting
- void processDeletedDirsForStore(SnapshotInfo currentSnapshotInfo,
KeyManager keyManager, long rnCnt, int remainNum)
+ void processDeletedDirsForStore(SnapshotInfo currentSnapshotInfo,
KeyManager keyManager,
Review Comment:
Extracted worker scheduling, handle closure, and completion waiting into
`runDeletionWorkers()`. `processDeletedDirsForStore()` now handles store setup
and final snapshot-property updates. Rejection and interruption handling remain
together in the coordination method.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -824,12 +920,25 @@ public BackgroundTaskResult call() {
} else if (!isPreviousPurgeTransactionFlushed()) {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot =
snapInfo == null ? null :
- omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(),
snapInfo.getBucketName(),
- snapInfo.getName())) {
- KeyManager keyManager = snapInfo == null ?
getOzoneManager().getKeyManager()
- : omSnapshot.get().getKeyManager();
- processDeletedDirsForStore(snapInfo, keyManager, run,
pathLimitPerTask);
+ boolean snapshotProcessingLockAcquired = false;
Review Comment:
Removed the lock-acquisition flag and nested cleanup blocks. With one store
coordinator, `call()` checks eligibility, opens the task-owned handle, and
invokes store processing. The deletion-worker pool retains configured
parallelism.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -658,43 +670,94 @@ void processDeletedDirsForStore(SnapshotInfo
currentSnapshotInfo, KeyManager key
UUID expectedPreviousSnapshotId = currentSnapshotInfo == null ?
snapshotChainManager.getLatestGlobalSnapshotId() :
SnapshotUtils.getPreviousSnapshotId(currentSnapshotInfo,
snapshotChainManager);
- Map<UUID, Pair<Long, Long>> exclusiveSizeMap = Maps.newConcurrentMap();
-
- CompletableFuture<Boolean> processedAllDeletedDirs =
CompletableFuture.completedFuture(true);
final int parallelThreads = numberOfParallelThreadsPerStore.get();
+ CountDownLatch snapshotDbHandlesClosed = currentSnapshotInfo == null ?
null :
Review Comment:
I added an AOS regression with three workers. One retains a snapshot DB read
lock while another waits for unflushed transaction capacity. The scanning
worker can still finish and release its handle, allowing both requests and the
flush to complete. The diagram needs an additional dependency preventing worker
B from finishing to form a deadlock. AOS therefore retains independent
per-worker handle closure before submission.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -824,12 +920,25 @@ public BackgroundTaskResult call() {
} else if (!isPreviousPurgeTransactionFlushed()) {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- try (UncheckedAutoCloseableSupplier<OmSnapshot> omSnapshot =
snapInfo == null ? null :
- omSnapshotManager.getActiveSnapshot(snapInfo.getVolumeName(),
snapInfo.getBucketName(),
- snapInfo.getName())) {
- KeyManager keyManager = snapInfo == null ?
getOzoneManager().getKeyManager()
- : omSnapshot.get().getKeyManager();
- processDeletedDirsForStore(snapInfo, keyManager, run,
pathLimitPerTask);
+ boolean snapshotProcessingLockAcquired = false;
Review Comment:
AOS did bypass the snapshot-task lock and share its deletion pool. The
revision uses one coordinator for both, so another store cannot open snapshot
handles until the current store’s workers finish. Workers within each store
remain parallel. The regression checks that the next coordinator task stays
queued during submission. The specific A/B/purge diagram still needs a
dependency stopping B’s scan to establish deadlock.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
