From 0f0309306f15d26ce4d2799424da4cd2a34d4d7b Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Thu, 15 May 2025 10:01:36 -0400 Subject: [PATCH 1/4] HDDS-13049. Deprecate VolumeName & BucketName in OmKeyPurgeRequest and prevent Key version purge on Block Deletion Failure Change-Id: I80dc0b5afc964332dd1056ace0e8690bf746e2a2 --- .../src/main/proto/OmClientProtocol.proto | 4 +- .../service/AbstractKeyDeletingService.java | 57 +++++++------------ .../om/service/TestKeyDeletingService.java | 41 +++++++++++++ 3 files changed, 62 insertions(+), 40 deletions(-) diff --git a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto index c7ff385016f7..b3a61db1941a 100644 --- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto +++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto @@ -1376,8 +1376,8 @@ message DeleteKeyResponse { } message DeletedKeys { - required string volumeName = 1; - required string bucketName = 2; + required string volumeName = 1 [deprecated = true]; + required string bucketName = 2 [deprecated = true]; repeated string keys = 3; } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java index dc03b25a99d5..0244f95aec1f 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java @@ -26,8 +26,10 @@ import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.UUID; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; @@ -63,7 +65,6 @@ import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type; import org.apache.hadoop.util.Time; import org.apache.ratis.protocol.ClientId; -import org.apache.ratis.util.Preconditions; /** * Abstracts common code from KeyDeletingService and DirectoryDeletingService @@ -144,29 +145,32 @@ protected int processKeyDeletes(List keyBlocksList, */ private int submitPurgeKeysRequest(List results, HashMap keysToModify, String snapTableKey, UUID expectedPreviousSnapshotId) { - Map, List> purgeKeysMapPerBucket = - new HashMap<>(); + List purgeKeys = new ArrayList<>(); // Put all keys to be purged in a list int deletedCount = 0; + Set failedDeletedKeys = new HashSet<>(); for (DeleteBlockGroupResult result : results) { + String deletedKey = result.getObjectKey(); if (result.isSuccess()) { // Add key to PurgeKeys list. - String deletedKey = result.getObjectKey(); if (keysToModify != null && !keysToModify.containsKey(deletedKey)) { // Parse Volume and BucketName - addToMap(purgeKeysMapPerBucket, deletedKey); + purgeKeys.add(deletedKey); if (LOG.isDebugEnabled()) { LOG.debug("Key {} set to be updated in OM DB, Other versions " + "of the key that are reclaimable are reclaimed.", deletedKey); } } else if (keysToModify == null) { - addToMap(purgeKeysMapPerBucket, deletedKey); + purgeKeys.add(deletedKey); if (LOG.isDebugEnabled()) { LOG.debug("Key {} set to be purged from OM DB", deletedKey); } } deletedCount++; + } else { + // If the block deletion failed, then the deleted keys should also not be modified. + failedDeletedKeys.add(deletedKey); } } @@ -180,24 +184,20 @@ private int submitPurgeKeysRequest(List results, expectedPreviousSnapshotNullableUUID.setUuid(HddsUtils.toProtobuf(expectedPreviousSnapshotId)); } purgeKeysRequest.setExpectedPreviousSnapshotID(expectedPreviousSnapshotNullableUUID.build()); - - // Add keys to PurgeKeysRequest bucket wise. - for (Map.Entry, List> entry : - purgeKeysMapPerBucket.entrySet()) { - Pair volumeBucketPair = entry.getKey(); - DeletedKeys deletedKeysInBucket = DeletedKeys.newBuilder() - .setVolumeName(volumeBucketPair.getLeft()) - .setBucketName(volumeBucketPair.getRight()) - .addAllKeys(entry.getValue()) - .build(); - purgeKeysRequest.addDeletedKeys(deletedKeysInBucket); - } + DeletedKeys deletedKeys = DeletedKeys.newBuilder() + .setVolumeName("") + .setBucketName("") + .addAllKeys(purgeKeys) + .build(); + purgeKeysRequest.addDeletedKeys(deletedKeys); List keysToUpdateList = new ArrayList<>(); if (keysToModify != null) { for (Map.Entry keyToModify : keysToModify.entrySet()) { - + if (failedDeletedKeys.contains(keyToModify.getKey())) { + continue; + } SnapshotMoveKeyInfos.Builder keyToUpdate = SnapshotMoveKeyInfos.newBuilder(); keyToUpdate.setKey(keyToModify.getKey()); @@ -235,25 +235,6 @@ protected OzoneManagerProtocolProtos.OMResponse submitRequest(OMRequest omReques return OzoneManagerRatisUtils.submitRequest(ozoneManager, omRequest, clientId, callId.incrementAndGet()); } - /** - * Parse Volume and Bucket Name from ObjectKey and add it to given map of - * keys to be purged per bucket. - */ - private void addToMap(Map, List> map, String objectKey) { - // Parse volume and bucket name - String[] split = objectKey.split(OM_KEY_PREFIX); - Preconditions.assertTrue(split.length >= 3, "Volume and/or Bucket Name " + - "missing from Key Name " + objectKey); - if (split.length == 3) { - LOG.warn("{} missing Key Name", objectKey); - } - Pair volumeBucketPair = Pair.of(split[1], split[2]); - if (!map.containsKey(volumeBucketPair)) { - map.put(volumeBucketPair, new ArrayList<>()); - } - map.get(volumeBucketPair).add(objectKey); - } - protected void submitPurgePaths(List requests, String snapTableKey, UUID expectedPreviousSnapshotId) { diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java index 20a91080c50c..3d66ad505dcd 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java @@ -28,8 +28,13 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.CALLS_REAL_METHODS; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableMap; @@ -46,7 +51,9 @@ import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import org.apache.commons.lang3.RandomStringUtils; +import org.apache.hadoop.hdds.client.BlockID; import org.apache.hadoop.hdds.client.RatisReplicationConfig; import org.apache.hadoop.hdds.client.StandaloneReplicationConfig; import org.apache.hadoop.hdds.conf.OzoneConfiguration; @@ -78,8 +85,10 @@ import org.apache.hadoop.ozone.om.helpers.RepeatedOmKeyInfo; import org.apache.hadoop.ozone.om.helpers.SnapshotInfo; import org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol; +import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerRatisUtils; import org.apache.hadoop.ozone.om.request.OMRequestTestUtils; import org.apache.hadoop.ozone.om.snapshot.ReferenceCounted; +import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos; import org.apache.ozone.test.GenericTestUtils; import org.apache.ozone.test.OzoneTestBase; import org.apache.ratis.util.ExitUtils; @@ -92,6 +101,7 @@ import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.io.TempDir; import org.mockito.ArgumentMatchers; +import org.mockito.MockedStatic; import org.mockito.Mockito; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -634,6 +644,37 @@ void cleanup() { } } + @Test + public void testFailingModifiedKeyPurge() throws IOException { + + try (MockedStatic mocked = mockStatic(OzoneManagerRatisUtils.class, + CALLS_REAL_METHODS)) { + AtomicReference purgeRequest = new AtomicReference<>(); + mocked.when(() -> OzoneManagerRatisUtils.submitRequest(any(), any(), any(), anyLong())) + .thenAnswer(i -> { + purgeRequest.set(i.getArgument(1)); + return OzoneManagerProtocolProtos.OMResponse.newBuilder().setCmdType(purgeRequest.get().getCmdType()) + .setStatus(OzoneManagerProtocolProtos.Status.TIMEOUT).build(); + }); + List blockGroups = Collections.singletonList(BlockGroup.newBuilder().setKeyName("key1") + .addAllBlockIDs(Collections.singletonList(new BlockID(1, 1))).build()); + OmKeyInfo omKeyInfo = new OmKeyInfo.Builder() + .setBucketName("buck") + .setVolumeName("vol") + .setKeyName("key1") + .setDataSize(10) + .setOmKeyLocationInfos(null) + .setReplicationConfig(RatisReplicationConfig.getInstance(THREE)) + .setObjectID(1) + .setParentObjectID(2) + .build(); + Map keysToModify = Collections.singletonMap("key1", + new RepeatedOmKeyInfo(Collections.singletonList(omKeyInfo))); + keyDeletingService.processKeyDeletes(blockGroups, keysToModify, null, null); + assertTrue(purgeRequest.get().getPurgeKeysRequest().getKeysToUpdateList().isEmpty()); + } + } + @Test void checkIfDeleteServiceWithFailingSCM() throws Exception { final int initialCount = countKeysPendingDeletion(); From 0d70657441d687334e494f8a41cbed9ae4bb0b89 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Sat, 17 May 2025 08:24:44 -0400 Subject: [PATCH 2/4] Update hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java Co-authored-by: Wei-Chiu Chuang --- .../apache/hadoop/ozone/om/service/TestKeyDeletingService.java | 1 + 1 file changed, 1 insertion(+) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java index 3d66ad505dcd..cd8fa51f5b62 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java @@ -645,6 +645,7 @@ void cleanup() { } @Test + @DisplayName("Should not update keys when purge request times out during key deletion") public void testFailingModifiedKeyPurge() throws IOException { try (MockedStatic mocked = mockStatic(OzoneManagerRatisUtils.class, From 50d06356992bed1b3894984f82244b737e394529 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Sat, 17 May 2025 11:42:47 -0400 Subject: [PATCH 3/4] HDDS-13049. Fix compilation issues Change-Id: I7016ac674d30a91975fc2128bcb5136501e618ad --- .../hadoop/ozone/om/service/AbstractKeyDeletingService.java | 5 ++--- .../apache/hadoop/ozone/om/service/KeyDeletingService.java | 3 +-- .../hadoop/ozone/om/service/TestKeyDeletingService.java | 1 + 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java index 0244f95aec1f..7e562dfb9113 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java @@ -103,8 +103,7 @@ public AbstractKeyDeletingService(String serviceName, long interval, } protected int processKeyDeletes(List keyBlocksList, - KeyManager manager, - HashMap keysToModify, + Map keysToModify, String snapTableKey, UUID expectedPreviousSnapshotId) throws IOException { long startTime = Time.monotonicNow(); @@ -144,7 +143,7 @@ protected int processKeyDeletes(List keyBlocksList, * @param keysToModify Updated list of RepeatedOmKeyInfo */ private int submitPurgeKeysRequest(List results, - HashMap keysToModify, String snapTableKey, UUID expectedPreviousSnapshotId) { + Map keysToModify, String snapTableKey, UUID expectedPreviousSnapshotId) { List purgeKeys = new ArrayList<>(); // Put all keys to be purged in a list diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java index 98d5a2f93c3c..04d5db31041f 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java @@ -220,7 +220,6 @@ public BackgroundTaskResult call() { .getKeyBlocksList(); if (keyBlocksList != null && !keyBlocksList.isEmpty()) { delCount = processKeyDeletes(keyBlocksList, - getOzoneManager().getKeyManager(), pendingKeysDeletion.getKeysToModify(), null, expectedPreviousSnapshotId); deletedKeyCount.addAndGet(delCount); metrics.incrNumKeysProcessed(keyBlocksList.size()); @@ -447,7 +446,7 @@ private void processSnapshotDeepClean(int delCount) } if (!keysToPurge.isEmpty()) { - processKeyDeletes(keysToPurge, currOmSnapshot.getKeyManager(), + processKeyDeletes(keysToPurge, keysToModify, currSnapInfo.getTableKey(), Optional.ofNullable(previousSnapshot).map(SnapshotInfo::getSnapshotId).orElse(null)); } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java index cd8fa51f5b62..46538492c6f4 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java @@ -96,6 +96,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; From a8134007e8949b1115a05d2d346639f99fe3f7db Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Sat, 17 May 2025 11:44:12 -0400 Subject: [PATCH 4/4] HDDS-13049. Fix checkstyle Change-Id: I1cf3808127043feada7167f0b3b8d296f9bfabdd --- .../hadoop/ozone/om/service/AbstractKeyDeletingService.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java index 7e562dfb9113..c0ab95b7741b 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java @@ -25,7 +25,6 @@ import com.google.protobuf.ServiceException; import java.io.IOException; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map;