Copilot commented on code in PR #11009:
URL: https://github.com/apache/ozone/pull/11009#discussion_r3772642279
##########
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:
`snapshotProcessingLock` is held for the entire snapshot deletion task
(including directory scanning and synchronous OM requests) even though the
comment says it only serializes snapshot setup. With the BackgroundService pool
size equal to the configured deletion threads, multiple snapshot tasks can end
up blocking on this lock and occupy the service thread pool, delaying AOS work
and other snapshots unnecessarily.
Consider limiting the lock scope to only the `getActiveSnapshot(...)` open
(or other setup that must be serialized) and releasing it immediately
afterward, before the potentially long-running
`processDeletedDirsForStore(...)`.
--
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]
