Copilot commented on code in PR #11216:
URL: https://github.com/apache/ozone/pull/11216#discussion_r3951322534
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java:
##########
@@ -247,7 +247,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager, Execut
keyArgs = validateAndRewriteIfMatchAsExpectedGeneration(keyArgs,
dbKeyInfo);
OmBucketInfo bucketInfo =
- getBucketInfo(omMetadataManager, volumeName, bucketName);
+ getBucketInfoForUpdate(omMetadataManager, volumeName, bucketName);
Review Comment:
When key-path locking is used (no missing parents), this path appears to
only read bucket info and never publishes it (because `numMissingParents` stays
0). In that case, `getBucketInfoForUpdate()` still creates an unnecessary copy.
If feasible, fetch read-only bucket info first and only switch to
`getBucketInfoForUpdate()` (copy + later publish) in the branch where missing
parents are actually created and charged.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -269,6 +278,18 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, Execut
getBucketLayout(), volBucketInfoMap, fromSnapshotInfo, openKeyInfoMap);
}
+ /**
+ * Return the copy this request accumulates deltas on, fetching it on first
use. All three purge
+ * paths can touch the same bucket, so they must share one copy or earlier
deltas are lost.
+ */
+ private OmBucketInfo accumulatedBucketInfo(Map<Pair<String, String>,
OmBucketInfo> volBucketInfoMap,
+ OMMetadataManager omMetadataManager, String volumeName, String
bucketName) {
+ OmBucketInfo omBucketInfo = volBucketInfoMap.get(Pair.of(volumeName,
bucketName));
+
+ return omBucketInfo != null ? omBucketInfo
+ : getBucketInfoForUpdate(omMetadataManager, volumeName, bucketName);
Review Comment:
This helper never inserts the newly fetched copy back into
`volBucketInfoMap`. As a result, subsequent calls for the same bucket will get
a fresh copy and earlier deltas can be lost; additionally, the publish loop
over `volBucketInfoMap.entrySet()` may publish nothing. Use `computeIfAbsent`
(or explicitly `put`) so the first fetched copy is retained and reused for all
purge paths.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java:
##########
@@ -333,6 +333,11 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, Execut
OMFileRequest.addKeyTableCacheEntries(omMetadataManager, volumeName,
bucketName, bucketInfo.getBucketLayout(),
null, missingParentInfos, trxnLogIndex);
+
+ // Parent directory creation holds the bucket write lock; key path
locking leaves
+ // numMissingParents at 0.
+ omMetadataManager.getBucketTable().addCacheEntry(
+ omMetadataManager.getBucketKey(volumeName, bucketName),
bucketInfo, trxnLogIndex);
Review Comment:
When key-path locking is used (no missing parents), this path appears to
only read bucket info and never publishes it (because `numMissingParents` stays
0). In that case, `getBucketInfoForUpdate()` still creates an unnecessary copy.
If feasible, fetch read-only bucket info first and only switch to
`getBucketInfoForUpdate()` (copy + later publish) in the branch where missing
parents are actually created and charged.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3ExpiredMultipartUploadsAbortRequest.java:
##########
@@ -106,18 +108,44 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, Execut
Result result = null;
Map<OmBucketInfo, List<OmMultipartAbortInfo>>
abortedMultipartUploads = new HashMap<>();
+ // One accumulated copy per bucket, so a bucket listed more than once
keeps a single entry.
+ Map<Pair<String, String>, OmBucketInfo> bucketInfoMap = new HashMap<>();
+
+ OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ List<String[]> bucketLockKeys = submittedExpiredMPUsPerBucket.stream()
+ .map(mpuByBucket -> Pair.of(mpuByBucket.getVolumeName(),
mpuByBucket.getBucketName()))
+ .distinct()
+ .map(volBucketPair -> new String[]{volBucketPair.getLeft(),
volBucketPair.getRight()})
+ .collect(Collectors.toList());
+ boolean acquiredLocks = false;
Review Comment:
The lock acquisition order is currently derived from request order. If
`acquireWriteLocks` does not internally enforce a deterministic ordering,
acquiring multiple bucket locks in varying orders can deadlock with concurrent
operations acquiring overlapping sets. Consider sorting `bucketLockKeys` (eg,
by volume then bucket) before acquiring to guarantee a consistent lock order.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyPurgeRequestAndResponse.java:
##########
@@ -137,6 +141,67 @@ private OMRequest preExecute(OMRequest originalOmRequest)
throws IOException {
return modifiedOmRequest;
}
+ @Test
+ public void testPurgedSizesReachCacheAndDb() throws Exception {
+ Pair<List<String>, List<String>> deleteKeysAndRenamedEntry =
+ createAndDeleteKeysAndRenamedEntry(1, null);
+
+ String bucketKey = omMetadataManager.getBucketKey(volumeName, bucketName);
+ OmBucketInfo bucketInfo = omMetadataManager.getBucketTable()
+ .getCacheValue(new CacheKey<>(bucketKey)).getCacheValue();
+ bucketInfo.incrSnapshotUsedBytes(500L);
+ bucketInfo.incrSnapshotUsedNamespace(5L);
Review Comment:
This test mutates the cached `OmBucketInfo` instance in-place to set up
state. Since this PR’s intent is to prevent in-place cache mutations, this
setup is a bit at odds with the new contract and may become brittle if bucket
table caching behavior changes. Prefer setting the initial bucket state via a
cache publication (`addCacheEntry`) or DB write so the test doesn’t rely on
mutating a shared cached instance.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3ExpiredMultipartUploadsAbortRequest.java:
##########
@@ -106,18 +108,44 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, Execut
Result result = null;
Map<OmBucketInfo, List<OmMultipartAbortInfo>>
abortedMultipartUploads = new HashMap<>();
+ // One accumulated copy per bucket, so a bucket listed more than once
keeps a single entry.
+ Map<Pair<String, String>, OmBucketInfo> bucketInfoMap = new HashMap<>();
+
+ OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ List<String[]> bucketLockKeys = submittedExpiredMPUsPerBucket.stream()
+ .map(mpuByBucket -> Pair.of(mpuByBucket.getVolumeName(),
mpuByBucket.getBucketName()))
+ .distinct()
+ .map(volBucketPair -> new String[]{volBucketPair.getLeft(),
volBucketPair.getRight()})
+ .collect(Collectors.toList());
+ boolean acquiredLocks = false;
try {
+ // Hold every bucket lock for the whole request, so the accumulated
copies can be published
+ // once all buckets have been processed. A later bucket failing turns
the whole request into
+ // an error response, which persists nothing.
+
mergeOmLockDetails(omMetadataManager.getLock().acquireWriteLocks(BUCKET_LOCK,
bucketLockKeys));
Review Comment:
The lock acquisition order is currently derived from request order. If
`acquireWriteLocks` does not internally enforce a deterministic ordering,
acquiring multiple bucket locks in varying orders can deadlock with concurrent
operations acquiring overlapping sets. Consider sorting `bucketLockKeys` (eg,
by volume then bucket) before acquiring to guarantee a consistent lock order.
--
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]
