diff --git a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto index b56912ff6d2c..fa7a5caf443a 100644 --- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto +++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto @@ -1380,8 +1380,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 7dcb696e06b3..8b9455b49c35 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,9 +25,10 @@ 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; +import java.util.Set; import java.util.UUID; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; @@ -63,7 +64,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 @@ -102,8 +102,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(); @@ -143,30 +142,33 @@ protected int processKeyDeletes(List keyBlocksList, * @param keysToModify Updated list of RepeatedOmKeyInfo */ private int submitPurgeKeysRequest(List results, - HashMap keysToModify, String snapTableKey, UUID expectedPreviousSnapshotId) { - Map, List> purgeKeysMapPerBucket = - new HashMap<>(); + Map keysToModify, String snapTableKey, UUID expectedPreviousSnapshotId) { + 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 +182,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 +233,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/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 20a91080c50c..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 @@ -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; @@ -87,11 +96,13 @@ 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; 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 +645,38 @@ 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, + 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();