You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "swamirishi (via GitHub)" <gi...@apache.org> on 2025/10/01 14:14:07 UTC
[PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
swamirishi opened a new pull request, #9084:
URL: https://github.com/apache/ozone/pull/9084
## What changes were proposed in this pull request?
Currently bucket locks are acquired in random bucket order(same order as the request) this could also have atomicity issue. Bulk locks should be acquired before applying the transaction.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/projects/HDDS/issues/HDDS-13729
## How was this patch tested?
Existing unit tests
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2396378877
##########
hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java:
##########
@@ -135,6 +136,15 @@ public interface OMMetadataManager extends DBStoreHAManager, AutoCloseable {
*/
String getBucketKeyPrefixFSO(String volume, String bucket) throws IOException;
+
+ /**
+ * Retrieves a pair of volume ID and bucket ID associated with the provided FSO (File System Object) key.
+ *
+ * @param fsoKey the key representing the File System Object, used to identify the corresponding volume and bucket.
+ * @return a Pair containing the volume ID as the first element and the bucket ID as the second element.
+ */
+ Pair<Long, Long> getVolumeBucketIdPairFSO(String fsoKey);
Review Comment:
Done added a new class
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Acquire Bulk Bucket locks in order to prevent deadlock in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #9084:
URL: https://github.com/apache/ozone/pull/9084#issuecomment-3368071857
Thank you @adoroszlai & @jojochuang for reviewing the patch.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Acquire Bulk Bucket locks in order to prevent deadlock in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi merged PR #9084:
URL: https://github.com/apache/ozone/pull/9084
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2402479394
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMDirectoriesPurgeRequestAndResponse.java:
##########
@@ -200,6 +220,120 @@ private OMRequest preExecute(OMRequest originalOmRequest) throws IOException {
return modifiedOmRequest;
}
+ private PurgePathRequest createBucketDataAndGetPurgePathRequest(OmBucketInfo bucketInfo) throws Exception {
+ OmDirectoryInfo dir1 = new OmDirectoryInfo.Builder()
+ .setName("dir1")
+ .setCreationTime(Time.now())
+ .setModificationTime(Time.now())
+ .setObjectID(1)
+ .setParentObjectID(bucketInfo.getObjectID())
+ .setUpdateID(0)
+ .build();
+ String dirKey = OMRequestTestUtils.addDirKeyToDirTable(false, dir1, volumeName,
+ bucketInfo.getBucketName(), 1L, omMetadataManager);
+ List<OmKeyInfo> subFiles = new ArrayList<>();
+ List<OmKeyInfo> subDirs = new ArrayList<>();
+ List<String> subFileKeys = new ArrayList<>();
+ List<String> subDirKeys = new ArrayList<>();
+ for (int id = 1; id < 10; id++) {
+ OmDirectoryInfo subdir = new OmDirectoryInfo.Builder()
+ .setName("subdir" + id)
+ .setCreationTime(Time.now())
+ .setModificationTime(Time.now())
+ .setObjectID(2 * id)
+ .setParentObjectID(dir1.getObjectID())
+ .setUpdateID(0)
+ .build();
+ String subDirectoryPath = OMRequestTestUtils.addDirKeyToDirTable(false, subdir, volumeName,
+ bucketInfo.getBucketName(), 2 * id, omMetadataManager);
+ subDirKeys.add(subDirectoryPath);
+ OmKeyInfo subFile =
+ OMRequestTestUtils.createOmKeyInfo(volumeName, bucketInfo.getBucketName(), "file" + id,
+ RatisReplicationConfig.getInstance(ONE))
+ .setObjectID(2 * id + 1)
+ .setParentObjectID(dir1.getObjectID())
+ .setUpdateID(100L)
+ .build();
+ String subFilePath = OMRequestTestUtils.addFileToKeyTable(false, true, subFile.getKeyName(),
+ subFile, 1234L, 2 * id + 1, omMetadataManager);
+ subFileKeys.add(subFilePath);
+ subFile.setKeyName("dir1/" + subFile.getKeyName());
+ subFiles.add(subFile);
+ subDirs.add(getOmKeyInfo(volumeName, bucketInfo.getBucketName(), subdir,
+ "dir1/" + subdir.getName()));
+ }
+ String deletedDirKey = OMRequestTestUtils.deleteDir(dirKey, volumeName, bucketInfo.getBucketName(),
+ omMetadataManager);
+ for (String subDirKey : subDirKeys) {
+ assertTrue(omMetadataManager.getDirectoryTable().isExist(subDirKey));
+ }
+ for (String subFileKey : subFileKeys) {
+ assertTrue(omMetadataManager.getFileTable().isExist(subFileKey));
+ }
+ assertFalse(omMetadataManager.getDirectoryTable().isExist(dirKey));
+ Long volumeId = omMetadataManager.getVolumeId(bucketInfo.getVolumeName());
+ long bucketId = bucketInfo.getObjectID();
+ return wrapPurgeRequest(volumeId, bucketId, deletedDirKey, subFiles, subDirs);
+ }
+
+ @Test
+ public void testBucketLockWithPurgeDirectory() throws Exception {
+ when(ozoneManager.getDefaultReplicationConfig())
+ .thenReturn(RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.THREE));
+ Random random = new Random();
+ String bucket1 = "bucket" + random.nextInt();
+ // Add volume, bucket and key entries to OM DB.
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucket1,
+ omMetadataManager, BucketLayout.FILE_SYSTEM_OPTIMIZED);
+ String bucketKey1 = omMetadataManager.getBucketKey(volumeName, bucket1);
+ OmBucketInfo bucketInfo1 = omMetadataManager.getBucketTable().get(bucketKey1);
+ PurgePathRequest purgePathRequest1 = createBucketDataAndGetPurgePathRequest(bucketInfo1);
+ String bucket2 = "bucket" + random.nextInt();
+ // Add volume, bucket and key entries to OM DB.
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucket2,
+ omMetadataManager, BucketLayout.FILE_SYSTEM_OPTIMIZED);
+ String bucketKey2 = omMetadataManager.getBucketKey(volumeName, bucket1);
+ OmBucketInfo bucketInfo2 = omMetadataManager.getBucketTable().get(bucketKey2);
+ PurgePathRequest purgePathRequest2 = createBucketDataAndGetPurgePathRequest(bucketInfo2);
+ IOzoneManagerLock lock = spy(omMetadataManager.getLock());
+ Set<Long> acquiredLockIds = new ConcurrentSkipListSet<>();
+ Set<String> acquiredLockKeys = new ConcurrentSkipListSet<>();
+ doAnswer(i -> {
+ long threadId = Thread.currentThread().getId();
+ GenericTestUtils.waitFor(() -> !acquiredLockIds.contains(threadId) || acquiredLockIds.size() == 2, 1000, 30000);
+ OMLockDetails lockDetails = (OMLockDetails) i.callRealMethod();
+ acquiredLockIds.add(threadId);
+ acquiredLockKeys.add(i.getArgument(1) + "/" + i.getArgument(2));
+ return lockDetails;
+ }).when(lock).acquireWriteLock(eq(BUCKET_LOCK), anyString(), anyString());
+
+ doAnswer(i -> {
+ long threadId = Thread.currentThread().getId();
+ GenericTestUtils.waitFor(() -> !acquiredLockIds.contains(threadId) || acquiredLockIds.size() == 2, 1000, 30000);
+ OMLockDetails lockDetails = (OMLockDetails) i.callRealMethod();
+ acquiredLockIds.add(threadId);
+ for (String[] lockKey : (List<String[]>) i.getArgument(1)) {
+ acquiredLockKeys.add(lockKey[0] + "/" + lockKey[1]);
+ }
+ return lockDetails;
+ }).when(lock).acquireWriteLocks(eq(BUCKET_LOCK), anyCollection());
+ when(omMetadataManager.getLock()).thenReturn(lock);
+ OMDirectoriesPurgeRequestWithFSO purgePathRequests1 = new OMDirectoriesPurgeRequestWithFSO(
+ preExecute(createPurgeKeysRequest(null, Arrays.asList(purgePathRequest1, purgePathRequest2))));
+ OMDirectoriesPurgeRequestWithFSO purgePathRequests2 = new OMDirectoriesPurgeRequestWithFSO(
+ preExecute(createPurgeKeysRequest(null, Arrays.asList(purgePathRequest2, purgePathRequest1))));
+ CompletableFuture future1 = CompletableFuture.runAsync(() -> purgePathRequests1.validateAndUpdateCache(ozoneManager,
+ 100L));
+ CompletableFuture future2 = CompletableFuture.runAsync(() -> purgePathRequests2.validateAndUpdateCache(ozoneManager,
+ 100L));
+ future1.get();
+ future2.get();
+ assertEquals(Stream.of(bucketInfo1.getVolumeName() + "/" + bucketInfo1.getBucketName(),
+ bucketInfo2.getVolumeName() + "/" + bucketInfo2.getBucketName()).collect(Collectors.toSet()),
+ acquiredLockKeys);
+ reset(lock);
Review Comment:
`reset(lock)` in `finally`?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -262,13 +264,18 @@ void optimizeDirDeletesAndSubmitRequest(
long remainingBufLimit, KeyManager keyManager,
CheckedFunction<KeyValue<String, OmKeyInfo>, Boolean, IOException> reclaimableDirChecker,
CheckedFunction<KeyValue<String, OmKeyInfo>, Boolean, IOException> reclaimableFileChecker,
+ Collection<BucketNameInfo> bucketInfos,
UUID expectedPreviousSnapshotId, long rnCnt) throws InterruptedException {
// Optimization to handle delete sub-dir and keys to remove quickly
// This case will be useful to handle when depth of directory is high
int subdirDelNum = 0;
int subDirRecursiveCnt = 0;
int consumedSize = 0;
+ Map<Pair<Long, Long>, BucketNameInfo> bucketNameInfoMap =
+ bucketInfos.stream().collect(Collectors.toMap(
+ bucketInfo -> Pair.of(bucketInfo.getVolumeId(), bucketInfo.getBucketId()),
Review Comment:
Please use `VolumeBucketId`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -544,6 +544,19 @@ public String getBucketKeyPrefixFSO(String volume, String bucket) throws IOExcep
return getOzoneKeyFSO(volume, bucket, OM_KEY_PREFIX);
}
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public VolumeBucketId getVolumeBucketIdPairFSO(String fsoKey) throws IOException {
+ String[] keySplit = fsoKey.split(OM_KEY_PREFIX);
+ try {
+ return new VolumeBucketId(Long.parseLong(keySplit[1]), Long.parseLong(keySplit[2]));
+ } catch (NumberFormatException e) {
+ throw new IOException("Invalid format for FSO Key: " + fsoKey, e);
+ }
+ }
Review Comment:
On another look, why is this being added to the `OMMetadataManager` interface in the first place? This looks like a small utility method, doesn't access any storage.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -613,12 +623,27 @@ private boolean processDeletedDirectories(SnapshotInfo currentSnapshotInfo, KeyM
long subFileNum = 0L;
int consumedSize = 0;
List<PurgePathRequest> purgePathRequestList = new ArrayList<>();
+ Map<Pair<String, String>, BucketNameInfo> bucketNameInfos = new HashMap<>();
+
List<Pair<String, OmKeyInfo>> allSubDirList = new ArrayList<>();
while (remainingBufLimit > 0) {
KeyValue<String, OmKeyInfo> pendingDeletedDirInfo = dirSupplier.get();
if (pendingDeletedDirInfo == null) {
break;
}
+ Pair<String, String> volumeBucketPair = Pair.of(pendingDeletedDirInfo.getValue().getVolumeName(),
+ pendingDeletedDirInfo.getValue().getBucketName());
Review Comment:
Please add local var for `pendingDeletedDirInfo.getValue()` to make the code more readable.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMDirectoriesPurgeRequestAndResponse.java:
##########
@@ -200,6 +220,120 @@ private OMRequest preExecute(OMRequest originalOmRequest) throws IOException {
return modifiedOmRequest;
}
+ private PurgePathRequest createBucketDataAndGetPurgePathRequest(OmBucketInfo bucketInfo) throws Exception {
+ OmDirectoryInfo dir1 = new OmDirectoryInfo.Builder()
+ .setName("dir1")
+ .setCreationTime(Time.now())
+ .setModificationTime(Time.now())
+ .setObjectID(1)
+ .setParentObjectID(bucketInfo.getObjectID())
+ .setUpdateID(0)
+ .build();
+ String dirKey = OMRequestTestUtils.addDirKeyToDirTable(false, dir1, volumeName,
+ bucketInfo.getBucketName(), 1L, omMetadataManager);
+ List<OmKeyInfo> subFiles = new ArrayList<>();
+ List<OmKeyInfo> subDirs = new ArrayList<>();
+ List<String> subFileKeys = new ArrayList<>();
+ List<String> subDirKeys = new ArrayList<>();
+ for (int id = 1; id < 10; id++) {
+ OmDirectoryInfo subdir = new OmDirectoryInfo.Builder()
+ .setName("subdir" + id)
+ .setCreationTime(Time.now())
+ .setModificationTime(Time.now())
+ .setObjectID(2 * id)
+ .setParentObjectID(dir1.getObjectID())
+ .setUpdateID(0)
+ .build();
+ String subDirectoryPath = OMRequestTestUtils.addDirKeyToDirTable(false, subdir, volumeName,
+ bucketInfo.getBucketName(), 2 * id, omMetadataManager);
+ subDirKeys.add(subDirectoryPath);
+ OmKeyInfo subFile =
+ OMRequestTestUtils.createOmKeyInfo(volumeName, bucketInfo.getBucketName(), "file" + id,
+ RatisReplicationConfig.getInstance(ONE))
+ .setObjectID(2 * id + 1)
+ .setParentObjectID(dir1.getObjectID())
+ .setUpdateID(100L)
+ .build();
+ String subFilePath = OMRequestTestUtils.addFileToKeyTable(false, true, subFile.getKeyName(),
+ subFile, 1234L, 2 * id + 1, omMetadataManager);
+ subFileKeys.add(subFilePath);
+ subFile.setKeyName("dir1/" + subFile.getKeyName());
+ subFiles.add(subFile);
+ subDirs.add(getOmKeyInfo(volumeName, bucketInfo.getBucketName(), subdir,
+ "dir1/" + subdir.getName()));
+ }
+ String deletedDirKey = OMRequestTestUtils.deleteDir(dirKey, volumeName, bucketInfo.getBucketName(),
+ omMetadataManager);
+ for (String subDirKey : subDirKeys) {
+ assertTrue(omMetadataManager.getDirectoryTable().isExist(subDirKey));
+ }
+ for (String subFileKey : subFileKeys) {
+ assertTrue(omMetadataManager.getFileTable().isExist(subFileKey));
+ }
+ assertFalse(omMetadataManager.getDirectoryTable().isExist(dirKey));
+ Long volumeId = omMetadataManager.getVolumeId(bucketInfo.getVolumeName());
+ long bucketId = bucketInfo.getObjectID();
+ return wrapPurgeRequest(volumeId, bucketId, deletedDirKey, subFiles, subDirs);
+ }
+
+ @Test
+ public void testBucketLockWithPurgeDirectory() throws Exception {
+ when(ozoneManager.getDefaultReplicationConfig())
+ .thenReturn(RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.THREE));
+ Random random = new Random();
+ String bucket1 = "bucket" + random.nextInt();
Review Comment:
Please use `RandomUtils.secure().randomInt()` instead of creating a `new Random()` instance.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2399470504
##########
hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java:
##########
@@ -675,4 +685,43 @@ String getMultipartKey(long volumeId, long bucketId,
*/
boolean containsIncompleteMPUs(String volume, String bucket)
throws IOException;
+
+ /**
+ * Represents a unique identifier for a specific bucket within a volume.
+ *
+ * This class combines a volume identifier and a bucket identifier
+ * to uniquely identify a bucket within a storage system.
+ */
+ class VolumeBucketId {
+ private final long volumeId;
Review Comment:
@swamirishi I'm confused here. I see this PR went through several iterations but the latest version only contains two files OMMetadataManager and OMDirectoriesPurgeRequestWithFSO. And it adds bucketId that is not used anywhere.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2395864230
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -252,4 +253,21 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
omResponse.build(), purgeRequests,
getBucketLayout(), volBucketInfoMap, fromSnapshotInfo, openKeyInfoMap);
}
+
+ private List<String[]> getBucketLockKeySet(PurgeDirectoriesRequest purgeDirsRequest) {
+ if (purgeDirsRequest.getBucketNameInfosList().isEmpty()) {
+ return purgeDirsRequest.getBucketNameInfosList().stream()
Review Comment:
Isn't this always an empty stream?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -544,6 +545,15 @@ public String getBucketKeyPrefixFSO(String volume, String bucket) throws IOExcep
return getOzoneKeyFSO(volume, bucket, OM_KEY_PREFIX);
}
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public Pair<Long, Long> getVolumeBucketIdPairFSO(String fsoKey) {
+ String[] keySplit = fsoKey.split(OM_KEY_PREFIX);
+ return Pair.of(Long.parseLong(keySplit[1]), Long.parseLong(keySplit[2]));
Review Comment:
Please handle exceptions related to array size and number parsing. Isn't there some method for parsing FSO keys already?
##########
hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java:
##########
@@ -135,6 +136,15 @@ public interface OMMetadataManager extends DBStoreHAManager, AutoCloseable {
*/
String getBucketKeyPrefixFSO(String volume, String bucket) throws IOException;
+
+ /**
+ * Retrieves a pair of volume ID and bucket ID associated with the provided FSO (File System Object) key.
+ *
+ * @param fsoKey the key representing the File System Object, used to identify the corresponding volume and bucket.
+ * @return a Pair containing the volume ID as the first element and the bucket ID as the second element.
+ */
+ Pair<Long, Long> getVolumeBucketIdPairFSO(String fsoKey);
Review Comment:
Please try to avoid using class from third-party library in interfaces, because it may require breaking change to evolve later. Also, `Pair` objects lose context when being passed around, `getLeft()` is very generic.
Please check if Ozone already has a domain class for volume+bucket ID pair, and add one if it doesn't.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #9084:
URL: https://github.com/apache/ozone/pull/9084#issuecomment-3366176599
@adoroszlai are you fine with the changes made?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2402725056
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -262,13 +264,18 @@ void optimizeDirDeletesAndSubmitRequest(
long remainingBufLimit, KeyManager keyManager,
CheckedFunction<KeyValue<String, OmKeyInfo>, Boolean, IOException> reclaimableDirChecker,
CheckedFunction<KeyValue<String, OmKeyInfo>, Boolean, IOException> reclaimableFileChecker,
+ Collection<BucketNameInfo> bucketInfos,
UUID expectedPreviousSnapshotId, long rnCnt) throws InterruptedException {
// Optimization to handle delete sub-dir and keys to remove quickly
// This case will be useful to handle when depth of directory is high
int subdirDelNum = 0;
int subDirRecursiveCnt = 0;
int consumedSize = 0;
+ Map<Pair<Long, Long>, BucketNameInfo> bucketNameInfoMap =
+ bucketInfos.stream().collect(Collectors.toMap(
+ bucketInfo -> Pair.of(bucketInfo.getVolumeId(), bucketInfo.getBucketId()),
Review Comment:
done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2394782027
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1438,6 +1438,14 @@ message PurgeDirectoriesRequest {
optional string snapshotTableKey = 2;
// previous snapshotID can also be null & this field would be absent in older requests.
optional NullableUUID expectedPreviousSnapshotID = 3;
+ repeated BucketNameInfo bucketNameInfos = 4;
+}
+
+message BucketNameInfo {
+ required uint64 volumeId = 1;
+ required uint64 bucketId = 2;
+ required string volumeName = 3;
+ required string bucketName = 4;
Review Comment:
Please don't add `required` members.
https://protobuf.dev/programming-guides/style/#required
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #9084:
URL: https://github.com/apache/ozone/pull/9084#issuecomment-3367789878
> overall looks good.
>
> But the two fields in BucketNameInfo are not used. Removing them still builds.
>
> ```
> diff --git a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
> index 01f4699a88a..2c91065ebb2 100644
> --- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
> +++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
> @@ -1442,8 +1442,8 @@ message PurgeDirectoriesRequest {
> }
>
> message BucketNameInfo {
> - optional uint64 volumeId = 1;
> - optional uint64 bucketId = 2;
> + //optional uint64 volumeId = 1;
> + //optional uint64 bucketId = 2;
> optional string volumeName = 3;
> optional string bucketName = 4;
> }
> diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java
> index 7abf1406d96..a915581abd1 100644
> --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java
> +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java
> @@ -632,8 +632,7 @@ private boolean processDeletedDirectories(SnapshotInfo currentSnapshotInfo, KeyM
> VolumeBucketId volumeBucketId =
> keyManager.getMetadataManager().getVolumeBucketIdPairFSO(pendingDeletedDirInfo.getKey());
> bucketNameInfos.computeIfAbsent(volumeBucketId,
> - (k) -> BucketNameInfo.newBuilder().setVolumeId(volumeBucketId.getVolumeId())
> - .setBucketId(volumeBucketId.getBucketId())
> + (k) -> BucketNameInfo.newBuilder()
> .setVolumeName(deletedDirInfo.getVolumeName())
> .setBucketName(deletedDirInfo.getBucketName())
> .build());
> ```
Yeah @jojochuang we need this for HDDS-13036 where we need to reduce snapshotUsedNamespace for purging a deleted directory. We only have volumeId and bucketId we need the reverse mapping to figure out the volume name and bucket name. We cannot do that without doing a redundant get on the deletedDirectoryTable which would be inefficient if there arew too many directories to be purged on the apply transaction as this would have to do a rocksdb get since DeletedDirectoryTable doesn't have a cache.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2396378361
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -252,4 +253,21 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
omResponse.build(), purgeRequests,
getBucketLayout(), volBucketInfoMap, fromSnapshotInfo, openKeyInfoMap);
}
+
+ private List<String[]> getBucketLockKeySet(PurgeDirectoriesRequest purgeDirsRequest) {
+ if (purgeDirsRequest.getBucketNameInfosList().isEmpty()) {
+ return purgeDirsRequest.getBucketNameInfosList().stream()
Review Comment:
yeah there was a typo in the condition.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2402724481
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -613,12 +623,27 @@ private boolean processDeletedDirectories(SnapshotInfo currentSnapshotInfo, KeyM
long subFileNum = 0L;
int consumedSize = 0;
List<PurgePathRequest> purgePathRequestList = new ArrayList<>();
+ Map<Pair<String, String>, BucketNameInfo> bucketNameInfos = new HashMap<>();
+
List<Pair<String, OmKeyInfo>> allSubDirList = new ArrayList<>();
while (remainingBufLimit > 0) {
KeyValue<String, OmKeyInfo> pendingDeletedDirInfo = dirSupplier.get();
if (pendingDeletedDirInfo == null) {
break;
}
+ Pair<String, String> volumeBucketPair = Pair.of(pendingDeletedDirInfo.getValue().getVolumeName(),
+ pendingDeletedDirInfo.getValue().getBucketName());
Review Comment:
done
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMDirectoriesPurgeRequestAndResponse.java:
##########
@@ -200,6 +220,120 @@ private OMRequest preExecute(OMRequest originalOmRequest) throws IOException {
return modifiedOmRequest;
}
+ private PurgePathRequest createBucketDataAndGetPurgePathRequest(OmBucketInfo bucketInfo) throws Exception {
+ OmDirectoryInfo dir1 = new OmDirectoryInfo.Builder()
+ .setName("dir1")
+ .setCreationTime(Time.now())
+ .setModificationTime(Time.now())
+ .setObjectID(1)
+ .setParentObjectID(bucketInfo.getObjectID())
+ .setUpdateID(0)
+ .build();
+ String dirKey = OMRequestTestUtils.addDirKeyToDirTable(false, dir1, volumeName,
+ bucketInfo.getBucketName(), 1L, omMetadataManager);
+ List<OmKeyInfo> subFiles = new ArrayList<>();
+ List<OmKeyInfo> subDirs = new ArrayList<>();
+ List<String> subFileKeys = new ArrayList<>();
+ List<String> subDirKeys = new ArrayList<>();
+ for (int id = 1; id < 10; id++) {
+ OmDirectoryInfo subdir = new OmDirectoryInfo.Builder()
+ .setName("subdir" + id)
+ .setCreationTime(Time.now())
+ .setModificationTime(Time.now())
+ .setObjectID(2 * id)
+ .setParentObjectID(dir1.getObjectID())
+ .setUpdateID(0)
+ .build();
+ String subDirectoryPath = OMRequestTestUtils.addDirKeyToDirTable(false, subdir, volumeName,
+ bucketInfo.getBucketName(), 2 * id, omMetadataManager);
+ subDirKeys.add(subDirectoryPath);
+ OmKeyInfo subFile =
+ OMRequestTestUtils.createOmKeyInfo(volumeName, bucketInfo.getBucketName(), "file" + id,
+ RatisReplicationConfig.getInstance(ONE))
+ .setObjectID(2 * id + 1)
+ .setParentObjectID(dir1.getObjectID())
+ .setUpdateID(100L)
+ .build();
+ String subFilePath = OMRequestTestUtils.addFileToKeyTable(false, true, subFile.getKeyName(),
+ subFile, 1234L, 2 * id + 1, omMetadataManager);
+ subFileKeys.add(subFilePath);
+ subFile.setKeyName("dir1/" + subFile.getKeyName());
+ subFiles.add(subFile);
+ subDirs.add(getOmKeyInfo(volumeName, bucketInfo.getBucketName(), subdir,
+ "dir1/" + subdir.getName()));
+ }
+ String deletedDirKey = OMRequestTestUtils.deleteDir(dirKey, volumeName, bucketInfo.getBucketName(),
+ omMetadataManager);
+ for (String subDirKey : subDirKeys) {
+ assertTrue(omMetadataManager.getDirectoryTable().isExist(subDirKey));
+ }
+ for (String subFileKey : subFileKeys) {
+ assertTrue(omMetadataManager.getFileTable().isExist(subFileKey));
+ }
+ assertFalse(omMetadataManager.getDirectoryTable().isExist(dirKey));
+ Long volumeId = omMetadataManager.getVolumeId(bucketInfo.getVolumeName());
+ long bucketId = bucketInfo.getObjectID();
+ return wrapPurgeRequest(volumeId, bucketId, deletedDirKey, subFiles, subDirs);
+ }
+
+ @Test
+ public void testBucketLockWithPurgeDirectory() throws Exception {
+ when(ozoneManager.getDefaultReplicationConfig())
+ .thenReturn(RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.THREE));
+ Random random = new Random();
+ String bucket1 = "bucket" + random.nextInt();
+ // Add volume, bucket and key entries to OM DB.
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucket1,
+ omMetadataManager, BucketLayout.FILE_SYSTEM_OPTIMIZED);
+ String bucketKey1 = omMetadataManager.getBucketKey(volumeName, bucket1);
+ OmBucketInfo bucketInfo1 = omMetadataManager.getBucketTable().get(bucketKey1);
+ PurgePathRequest purgePathRequest1 = createBucketDataAndGetPurgePathRequest(bucketInfo1);
+ String bucket2 = "bucket" + random.nextInt();
+ // Add volume, bucket and key entries to OM DB.
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucket2,
+ omMetadataManager, BucketLayout.FILE_SYSTEM_OPTIMIZED);
+ String bucketKey2 = omMetadataManager.getBucketKey(volumeName, bucket1);
+ OmBucketInfo bucketInfo2 = omMetadataManager.getBucketTable().get(bucketKey2);
+ PurgePathRequest purgePathRequest2 = createBucketDataAndGetPurgePathRequest(bucketInfo2);
+ IOzoneManagerLock lock = spy(omMetadataManager.getLock());
+ Set<Long> acquiredLockIds = new ConcurrentSkipListSet<>();
+ Set<String> acquiredLockKeys = new ConcurrentSkipListSet<>();
+ doAnswer(i -> {
+ long threadId = Thread.currentThread().getId();
+ GenericTestUtils.waitFor(() -> !acquiredLockIds.contains(threadId) || acquiredLockIds.size() == 2, 1000, 30000);
+ OMLockDetails lockDetails = (OMLockDetails) i.callRealMethod();
+ acquiredLockIds.add(threadId);
+ acquiredLockKeys.add(i.getArgument(1) + "/" + i.getArgument(2));
+ return lockDetails;
+ }).when(lock).acquireWriteLock(eq(BUCKET_LOCK), anyString(), anyString());
+
+ doAnswer(i -> {
+ long threadId = Thread.currentThread().getId();
+ GenericTestUtils.waitFor(() -> !acquiredLockIds.contains(threadId) || acquiredLockIds.size() == 2, 1000, 30000);
+ OMLockDetails lockDetails = (OMLockDetails) i.callRealMethod();
+ acquiredLockIds.add(threadId);
+ for (String[] lockKey : (List<String[]>) i.getArgument(1)) {
+ acquiredLockKeys.add(lockKey[0] + "/" + lockKey[1]);
+ }
+ return lockDetails;
+ }).when(lock).acquireWriteLocks(eq(BUCKET_LOCK), anyCollection());
+ when(omMetadataManager.getLock()).thenReturn(lock);
+ OMDirectoriesPurgeRequestWithFSO purgePathRequests1 = new OMDirectoriesPurgeRequestWithFSO(
+ preExecute(createPurgeKeysRequest(null, Arrays.asList(purgePathRequest1, purgePathRequest2))));
+ OMDirectoriesPurgeRequestWithFSO purgePathRequests2 = new OMDirectoriesPurgeRequestWithFSO(
+ preExecute(createPurgeKeysRequest(null, Arrays.asList(purgePathRequest2, purgePathRequest1))));
+ CompletableFuture future1 = CompletableFuture.runAsync(() -> purgePathRequests1.validateAndUpdateCache(ozoneManager,
+ 100L));
+ CompletableFuture future2 = CompletableFuture.runAsync(() -> purgePathRequests2.validateAndUpdateCache(ozoneManager,
+ 100L));
+ future1.get();
+ future2.get();
+ assertEquals(Stream.of(bucketInfo1.getVolumeName() + "/" + bucketInfo1.getBucketName(),
+ bucketInfo2.getVolumeName() + "/" + bucketInfo2.getBucketName()).collect(Collectors.toSet()),
+ acquiredLockKeys);
+ reset(lock);
Review Comment:
done
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMDirectoriesPurgeRequestAndResponse.java:
##########
@@ -200,6 +220,120 @@ private OMRequest preExecute(OMRequest originalOmRequest) throws IOException {
return modifiedOmRequest;
}
+ private PurgePathRequest createBucketDataAndGetPurgePathRequest(OmBucketInfo bucketInfo) throws Exception {
+ OmDirectoryInfo dir1 = new OmDirectoryInfo.Builder()
+ .setName("dir1")
+ .setCreationTime(Time.now())
+ .setModificationTime(Time.now())
+ .setObjectID(1)
+ .setParentObjectID(bucketInfo.getObjectID())
+ .setUpdateID(0)
+ .build();
+ String dirKey = OMRequestTestUtils.addDirKeyToDirTable(false, dir1, volumeName,
+ bucketInfo.getBucketName(), 1L, omMetadataManager);
+ List<OmKeyInfo> subFiles = new ArrayList<>();
+ List<OmKeyInfo> subDirs = new ArrayList<>();
+ List<String> subFileKeys = new ArrayList<>();
+ List<String> subDirKeys = new ArrayList<>();
+ for (int id = 1; id < 10; id++) {
+ OmDirectoryInfo subdir = new OmDirectoryInfo.Builder()
+ .setName("subdir" + id)
+ .setCreationTime(Time.now())
+ .setModificationTime(Time.now())
+ .setObjectID(2 * id)
+ .setParentObjectID(dir1.getObjectID())
+ .setUpdateID(0)
+ .build();
+ String subDirectoryPath = OMRequestTestUtils.addDirKeyToDirTable(false, subdir, volumeName,
+ bucketInfo.getBucketName(), 2 * id, omMetadataManager);
+ subDirKeys.add(subDirectoryPath);
+ OmKeyInfo subFile =
+ OMRequestTestUtils.createOmKeyInfo(volumeName, bucketInfo.getBucketName(), "file" + id,
+ RatisReplicationConfig.getInstance(ONE))
+ .setObjectID(2 * id + 1)
+ .setParentObjectID(dir1.getObjectID())
+ .setUpdateID(100L)
+ .build();
+ String subFilePath = OMRequestTestUtils.addFileToKeyTable(false, true, subFile.getKeyName(),
+ subFile, 1234L, 2 * id + 1, omMetadataManager);
+ subFileKeys.add(subFilePath);
+ subFile.setKeyName("dir1/" + subFile.getKeyName());
+ subFiles.add(subFile);
+ subDirs.add(getOmKeyInfo(volumeName, bucketInfo.getBucketName(), subdir,
+ "dir1/" + subdir.getName()));
+ }
+ String deletedDirKey = OMRequestTestUtils.deleteDir(dirKey, volumeName, bucketInfo.getBucketName(),
+ omMetadataManager);
+ for (String subDirKey : subDirKeys) {
+ assertTrue(omMetadataManager.getDirectoryTable().isExist(subDirKey));
+ }
+ for (String subFileKey : subFileKeys) {
+ assertTrue(omMetadataManager.getFileTable().isExist(subFileKey));
+ }
+ assertFalse(omMetadataManager.getDirectoryTable().isExist(dirKey));
+ Long volumeId = omMetadataManager.getVolumeId(bucketInfo.getVolumeName());
+ long bucketId = bucketInfo.getObjectID();
+ return wrapPurgeRequest(volumeId, bucketId, deletedDirKey, subFiles, subDirs);
+ }
+
+ @Test
+ public void testBucketLockWithPurgeDirectory() throws Exception {
+ when(ozoneManager.getDefaultReplicationConfig())
+ .thenReturn(RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.THREE));
+ Random random = new Random();
+ String bucket1 = "bucket" + random.nextInt();
Review Comment:
done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2403677362
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1438,6 +1438,14 @@ message PurgeDirectoriesRequest {
optional string snapshotTableKey = 2;
// previous snapshotID can also be null & this field would be absent in older requests.
optional NullableUUID expectedPreviousSnapshotID = 3;
+ repeated BucketNameInfo bucketNameInfos = 4;
+}
+
+message BucketNameInfo {
+ optional uint64 volumeId = 1;
+ optional uint64 bucketId = 2;
Review Comment:
are these two fields needed? doesn't look so to me.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #9084:
URL: https://github.com/apache/ozone/pull/9084#issuecomment-3367796611
@jojochuang take a look at this commit in the PR explaining the usage
https://github.com/apache/ozone/pull/8587/commits/ea4c3ee381de3cd11cbf0ad1373b4ab8635538ad
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2399470504
##########
hadoop-ozone/interface-storage/src/main/java/org/apache/hadoop/ozone/om/OMMetadataManager.java:
##########
@@ -675,4 +685,43 @@ String getMultipartKey(long volumeId, long bucketId,
*/
boolean containsIncompleteMPUs(String volume, String bucket)
throws IOException;
+
+ /**
+ * Represents a unique identifier for a specific bucket within a volume.
+ *
+ * This class combines a volume identifier and a bucket identifier
+ * to uniquely identify a bucket within a storage system.
+ */
+ class VolumeBucketId {
+ private final long volumeId;
Review Comment:
@swamirishi I'm confused here. I see this PR went through several iterations but the latest version only contains two files OMMetadataManager and OMDirectoriesPurgeRequestWithFSO. And it adds bucketId that is not used anywhere.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2394795442
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1438,6 +1438,14 @@ message PurgeDirectoriesRequest {
optional string snapshotTableKey = 2;
// previous snapshotID can also be null & this field would be absent in older requests.
optional NullableUUID expectedPreviousSnapshotID = 3;
+ repeated BucketNameInfo bucketNameInfos = 4;
+}
+
+message BucketNameInfo {
+ required uint64 volumeId = 1;
+ required uint64 bucketId = 2;
+ required string volumeName = 3;
+ required string bucketName = 4;
Review Comment:
done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #9084:
URL: https://github.com/apache/ozone/pull/9084#issuecomment-3358665167
> > Existing unit tests
>
> Which ones test for the deadlock and how did they pass previously?
Currently we have tests which submit ratis requests in parallel however since validateAndUpdateCache is completely synchronized and runs in a single thread there is no multi threadig that runs on the method. I added a test case which should simulate the deadlock condition
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2402597585
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -544,6 +544,19 @@ public String getBucketKeyPrefixFSO(String volume, String bucket) throws IOExcep
return getOzoneKeyFSO(volume, bucket, OM_KEY_PREFIX);
}
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public VolumeBucketId getVolumeBucketIdPairFSO(String fsoKey) throws IOException {
+ String[] keySplit = fsoKey.split(OM_KEY_PREFIX);
+ try {
+ return new VolumeBucketId(Long.parseLong(keySplit[1]), Long.parseLong(keySplit[2]));
+ } catch (NumberFormatException e) {
+ throw new IOException("Invalid format for FSO Key: " + fsoKey, e);
+ }
+ }
Review Comment:
this needs to be OmMetadataManager interface because the implementation of OmMetadataManager i.e. OmMetadataManagerImpl should dictate how the entries are layed out. We should have this in an interface instead of a util function
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2396379980
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -544,6 +545,15 @@ public String getBucketKeyPrefixFSO(String volume, String bucket) throws IOExcep
return getOzoneKeyFSO(volume, bucket, OM_KEY_PREFIX);
}
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public Pair<Long, Long> getVolumeBucketIdPairFSO(String fsoKey) {
+ String[] keySplit = fsoKey.split(OM_KEY_PREFIX);
+ return Pair.of(Long.parseLong(keySplit[1]), Long.parseLong(keySplit[2]));
Review Comment:
No this is the only place. However I am throwing a checked exception if there is an exception while parsing
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2402597585
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -544,6 +544,19 @@ public String getBucketKeyPrefixFSO(String volume, String bucket) throws IOExcep
return getOzoneKeyFSO(volume, bucket, OM_KEY_PREFIX);
}
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public VolumeBucketId getVolumeBucketIdPairFSO(String fsoKey) throws IOException {
+ String[] keySplit = fsoKey.split(OM_KEY_PREFIX);
+ try {
+ return new VolumeBucketId(Long.parseLong(keySplit[1]), Long.parseLong(keySplit[2]));
+ } catch (NumberFormatException e) {
+ throw new IOException("Invalid format for FSO Key: " + fsoKey, e);
+ }
+ }
Review Comment:
this needs to be OmMetadataManager interface because the implementation of OmMetadataManager i.e. OmMetadataManagerImpl should dictate how the entries are layed out. We should not have this in an interface
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2403663397
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -239,17 +242,32 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
AUDIT.logWriteFailure(ozoneManager.buildAuditMessageForFailure(OMSystemAction.DIRECTORY_DELETION, null, ex));
throw new IllegalStateException(ex);
} finally {
- lockSet.stream().forEach(e -> omMetadataManager.getLock()
- .releaseWriteLock(BUCKET_LOCK, e.getKey(),
- e.getValue()));
for (Map.Entry<Pair<String, String>, OmBucketInfo> entry :
volBucketInfoMap.entrySet()) {
entry.setValue(entry.getValue().copyObject());
}
+ omMetadataManager.getLock().releaseWriteLocks(BUCKET_LOCK, bucketLockKeys);
}
return new OMDirectoriesPurgeResponseWithFSO(
omResponse.build(), purgeRequests,
getBucketLayout(), volBucketInfoMap, fromSnapshotInfo, openKeyInfoMap);
}
+
+ private List<String[]> getBucketLockKeySet(PurgeDirectoriesRequest purgeDirsRequest) {
+ if (!purgeDirsRequest.getBucketNameInfosList().isEmpty()) {
Review Comment:
in this case, does it need not to consider deletedSubFilesList?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13729. Deadlock possiblity in OmDirectoryPurgeRequest [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9084:
URL: https://github.com/apache/ozone/pull/9084#discussion_r2403663397
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -239,17 +242,32 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
AUDIT.logWriteFailure(ozoneManager.buildAuditMessageForFailure(OMSystemAction.DIRECTORY_DELETION, null, ex));
throw new IllegalStateException(ex);
} finally {
- lockSet.stream().forEach(e -> omMetadataManager.getLock()
- .releaseWriteLock(BUCKET_LOCK, e.getKey(),
- e.getValue()));
for (Map.Entry<Pair<String, String>, OmBucketInfo> entry :
volBucketInfoMap.entrySet()) {
entry.setValue(entry.getValue().copyObject());
}
+ omMetadataManager.getLock().releaseWriteLocks(BUCKET_LOCK, bucketLockKeys);
}
return new OMDirectoriesPurgeResponseWithFSO(
omResponse.build(), purgeRequests,
getBucketLayout(), volBucketInfoMap, fromSnapshotInfo, openKeyInfoMap);
}
+
+ private List<String[]> getBucketLockKeySet(PurgeDirectoriesRequest purgeDirsRequest) {
+ if (!purgeDirsRequest.getBucketNameInfosList().isEmpty()) {
Review Comment:
in this case, does it need not to consider deletedSubFilesList?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org