From 5fe2ff77c982fcb2fd910cb5ce61c8b5dece2ec9 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Tue, 12 Mar 2024 21:14:13 +0000 Subject: [PATCH 01/26] Add existing update / object IDs to OmKeyArgs and OmKeyInfo --- .../hadoop/ozone/om/helpers/OmKeyArgs.java | 45 +++++++++++++-- .../hadoop/ozone/om/helpers/OmKeyInfo.java | 56 +++++++++++++++++++ .../ozone/om/helpers/TestOmKeyInfo.java | 4 ++ .../src/main/proto/OmClientProtocol.proto | 14 +++++ 4 files changed, 115 insertions(+), 4 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java index 132c39c4d00e..7a7b4c8000dd 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java @@ -52,6 +52,13 @@ public final class OmKeyArgs implements Auditable { private final boolean recursive; private final boolean headOp; private final boolean forceUpdateContainerCacheFromSCM; + // Overwrite objectID and updateID when used in key creation indicate that a + // key with the same keyName should exist with the given object and update IDs, + // and upon commit, the object and updateID of that existing key should be unchanged. + // This is a form of optimistic locking, to ensure the key is not modified between + // the time the key is read and the time the key is written. + private Long overwriteObjectID = null; + private Long overwriteUpdateID = null; private OmKeyArgs(Builder b) { this.volumeName = b.volumeName; @@ -70,6 +77,8 @@ private OmKeyArgs(Builder b) { this.recursive = b.recursive; this.headOp = b.headOp; this.forceUpdateContainerCacheFromSCM = b.forceUpdateContainerCacheFromSCM; + this.overwriteObjectID = b.overwriteObjectID; + this.overwriteUpdateID = b.overwriteUpdateID; } public boolean getIsMultipartKey() { @@ -144,6 +153,14 @@ public boolean isForceUpdateContainerCacheFromSCM() { return forceUpdateContainerCacheFromSCM; } + public Long getOverwriteObjectID() { + return overwriteObjectID; + } + + public Long getOverwriteUpdateID() { + return overwriteUpdateID; + } + @Override public Map toAuditMap() { Map auditMap = new LinkedHashMap<>(); @@ -181,12 +198,14 @@ public OmKeyArgs.Builder toBuilder() { .setHeadOp(headOp) .setLatestVersionLocation(latestVersionLocation) .setAcls(acls) - .setForceUpdateContainerCacheFromSCM(forceUpdateContainerCacheFromSCM); + .setForceUpdateContainerCacheFromSCM(forceUpdateContainerCacheFromSCM) + .setOverwriteUpdateID(overwriteUpdateID) + .setOverwriteObjectID(overwriteObjectID); } @Nonnull public KeyArgs toProtobuf() { - return KeyArgs.newBuilder() + KeyArgs.Builder builder = KeyArgs.newBuilder() .setVolumeName(getVolumeName()) .setBucketName(getBucketName()) .setKeyName(getKeyName()) @@ -195,8 +214,14 @@ public KeyArgs toProtobuf() { .setLatestVersionLocation(getLatestVersionLocation()) .setHeadOp(isHeadOp()) .setForceUpdateContainerCacheFromSCM( - isForceUpdateContainerCacheFromSCM()) - .build(); + isForceUpdateContainerCacheFromSCM()); + if (overwriteUpdateID != null) { + toBuilder().setOverwriteObjectID(overwriteUpdateID); + } + if (overwriteUpdateID != null) { + toBuilder().setOverwriteUpdateID(overwriteUpdateID); + } + return builder.build(); } /** @@ -219,6 +244,8 @@ public static class Builder { private boolean recursive; private boolean headOp; private boolean forceUpdateContainerCacheFromSCM; + private Long overwriteObjectID = null; + private Long overwriteUpdateID = null; public Builder setVolumeName(String volume) { this.volumeName = volume; @@ -313,6 +340,16 @@ public Builder setForceUpdateContainerCacheFromSCM(boolean value) { return this; } + public Builder setOverwriteObjectID(long overwriteObjectID) { + this.overwriteObjectID = overwriteObjectID; + return this; + } + + public Builder setOverwriteUpdateID(long overwriteUpdateID) { + this.overwriteUpdateID = overwriteUpdateID; + return this; + } + public OmKeyArgs build() { return new OmKeyArgs(this); } diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java index e1b7ce1f27af..d0f625fb5123 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java @@ -100,6 +100,14 @@ public static Codec getCodec(boolean ignorePipeline) { */ private final List acls; + // Overwrite objectID and updateID when used in key creation indicate that a + // key with the same keyName should exist with the given object and update IDs, + // and upon commit, the object and updateID of that existing key should be unchanged. + // This is a form of optimistic locking, to ensure the key is not modified between + // the time the key is read and the time the key is written. + private Long overwriteObjectID = null; + private Long overwriteUpdateID = null; + private OmKeyInfo(Builder b) { setMetadata(b.metadata); setObjectID(b.objectID); @@ -118,6 +126,8 @@ private OmKeyInfo(Builder b) { this.fileChecksum = b.fileChecksum; this.fileName = b.fileName; this.isFile = b.isFile; + this.overwriteObjectID = b.overwriteObjectID; + this.overwriteUpdateID = b.overwriteUpdateID; } public String getVolumeName() { @@ -160,6 +170,22 @@ public String getFileName() { return fileName; } + public void setExistingObjectID(Long existingObjectID) { + this.overwriteObjectID = existingObjectID; + } + + public Long getOverwriteObjectID() { + return overwriteObjectID; + } + + public void setOverwriteUpdateID(Long existingUpdateID) { + this.overwriteUpdateID = existingUpdateID; + } + + public Long getOverwriteUpdateID() { + return overwriteUpdateID; + } + public synchronized OmKeyLocationInfoGroup getLatestVersionLocations() { return keyLocationVersions.size() == 0 ? null : keyLocationVersions.get(keyLocationVersions.size() - 1); @@ -434,6 +460,8 @@ public static class Builder { private FileChecksum fileChecksum; private boolean isFile; + private Long overwriteObjectID = null; + private Long overwriteUpdateID = null; public Builder() { this.metadata = new HashMap<>(); @@ -550,6 +578,16 @@ public Builder setFile(boolean isAFile) { return this; } + public Builder setOverwriteObjectID(Long existingObjectID) { + this.overwriteObjectID = existingObjectID; + return this; + } + + public Builder setOverwriteUpdateID(Long existingUpdateID) { + this.overwriteUpdateID = existingUpdateID; + return this; + } + public OmKeyInfo build() { return new OmKeyInfo(this); } @@ -654,6 +692,12 @@ private KeyInfo getProtobuf(boolean ignorePipeline, String fullKeyName, kb.setFileEncryptionInfo(OMPBHelper.convert(encInfo)); } kb.setIsFile(isFile); + if (overwriteUpdateID != null) { + kb.setOverwriteUpdateID(overwriteUpdateID); + } + if (overwriteObjectID != null) { + kb.setOverwriteObjectID(overwriteObjectID); + } return kb.build(); } @@ -700,6 +744,12 @@ public static OmKeyInfo getFromProtobuf(KeyInfo keyInfo) throws IOException { if (keyInfo.hasIsFile()) { builder.setFile(keyInfo.getIsFile()); } + if (keyInfo.hasOverwriteObjectID()) { + builder.setOverwriteObjectID(keyInfo.getOverwriteObjectID()); + } + if (keyInfo.hasOverwriteUpdateID()) { + builder.setOverwriteUpdateID(keyInfo.getOverwriteUpdateID()); + } // not persisted to DB. FileName will be filtered out from keyName builder.setFileName(OzoneFSUtils.getFileName(keyInfo.getKeyName())); @@ -806,6 +856,12 @@ public OmKeyInfo copyObject() { if (fileChecksum != null) { builder.setFileChecksum(fileChecksum); } + if (overwriteObjectID != null) { + builder.setOverwriteObjectID(overwriteObjectID); + } + if (overwriteUpdateID != null) { + builder.setOverwriteUpdateID(overwriteUpdateID); + } return builder.build(); } diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java index 6396f0318dcc..62b1d634f625 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java @@ -67,6 +67,8 @@ public void protobufConversion() throws IOException { assertFalse(key.isHsync()); key.getMetadata().put(OzoneConsts.HSYNC_CLIENT_ID, "clientid"); assertTrue(key.isHsync()); + assertEquals(1234L, key.getOverwriteObjectID()); + assertEquals(5678L, key.getOverwriteUpdateID()); } @Test @@ -123,6 +125,8 @@ private OmKeyInfo createOmKeyInfo(ReplicationConfig replicationConfig) { .setReplicationConfig(replicationConfig) .addMetadata("key1", "value1") .addMetadata("key2", "value2") + .setOverwriteObjectID(1234L) + .setOverwriteUpdateID(5678L) .build(); } diff --git a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto index b0d26020c8d2..39e29144da0c 100644 --- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto +++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto @@ -1025,6 +1025,13 @@ message KeyArgs { optional hadoop.hdds.ECReplicationConfig ecReplicationConfig = 19; // Force OM to update container cache location from SCL optional bool forceUpdateContainerCacheFromSCM = 20; + // Overwrite objectID and updateID when used in key creation indicate that a + // key with the same keyName should exist with the given object and update IDs, + // and upon commit, the object and updateID of that existing key should be unchanged. + // This is a form of optimistic locking, to ensure the key is not modified between + // the time the key is read and the time the key is written. + optional uint64 overwriteObjectID = 21; + optional uint64 overwriteUpdateID = 22; } message KeyLocation { @@ -1107,6 +1114,13 @@ message KeyInfo { optional hadoop.hdds.ECReplicationConfig ecReplicationConfig = 17; optional FileChecksumProto fileChecksum = 18; optional bool isFile = 19; + // Overwrite objectID and updateID when used in key creation indicate that a + // key with the same keyName should exist with the given object and update IDs, + // and upon commit, the object and updateID of that existing key should be unchanged. + // This is a form of optimistic locking, to ensure the key is not modified between + // the time the key is read and the time the key is written. + optional uint64 overwriteObjectID = 20; + optional uint64 overwriteUpdateID = 21; } message BasicKeyInfo { From aa3bd32827ad1c6e0443ff414ca54fac6c4c97e5 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Wed, 13 Mar 2024 20:22:00 +0000 Subject: [PATCH 02/26] modified createKeyRequest and commitKeyRequest with tests --- .../hadoop/ozone/om/helpers/OmKeyInfo.java | 8 +- .../om/request/key/OMKeyCommitRequest.java | 24 ++++++ .../om/request/key/OMKeyCreateRequest.java | 19 +++++ .../ozone/om/request/key/OMKeyRequest.java | 10 +++ .../request/key/TestOMKeyCommitRequest.java | 80 +++++++++++++++++-- .../request/key/TestOMKeyCreateRequest.java | 70 +++++++++++++++- 6 files changed, 198 insertions(+), 13 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java index d0f625fb5123..c478e943a8ad 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java @@ -170,16 +170,16 @@ public String getFileName() { return fileName; } - public void setExistingObjectID(Long existingObjectID) { - this.overwriteObjectID = existingObjectID; + public void setOverwriteObjectID(Long overwriteObjectID) { + this.overwriteObjectID = overwriteObjectID; } public Long getOverwriteObjectID() { return overwriteObjectID; } - public void setOverwriteUpdateID(Long existingUpdateID) { - this.overwriteUpdateID = existingUpdateID; + public void setOverwriteUpdateID(Long overwriteUpdateID) { + this.overwriteUpdateID = overwriteUpdateID; } public Long getOverwriteUpdateID() { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java index b28b390efd73..2a9a4093e95c 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java @@ -240,6 +240,13 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn throw new OMException("Failed to " + action + " key, as " + dbOpenKey + "entry is not found in the OpenKey table", KEY_NOT_FOUND); } + + validateOptimisticLockingOverwrite(keyToDelete, omKeyInfo); + // Optimistic locking validation has passed. Now set the overwrite fields to null so they are + // not persisted in the key table. + omKeyInfo.setOverwriteUpdateID(null); + omKeyInfo.setOverwriteObjectID(null); + omKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( commitKeyArgs.getMetadataList())); if (isHSync) { @@ -497,4 +504,21 @@ public static OMRequest disallowHsync( } return req; } + + private void validateOptimisticLockingOverwrite(OmKeyInfo existing, OmKeyInfo toCommit) + throws OMException { + if (toCommit.getOverwriteObjectID() != null || toCommit.getOverwriteUpdateID() != null) { + if (existing == null) { + throw new OMException("Overwrite with optimistic locking is not allowed for a new key", KEY_NOT_FOUND); + } + if (!toCommit.getOverwriteObjectID().equals(existing.getObjectID()) || + !toCommit.getOverwriteUpdateID().equals(existing.getUpdateID())) { + throw new OMException("Cannot commit as current objectID / updateID (" + + existing.getObjectID() + " / " + existing.getUpdateID() + + ") does not match with the overwrite objectID / updateID (" + + toCommit.getOverwriteObjectID() + " / " + toCommit.getOverwriteUpdateID() + ")", KEY_NOT_FOUND); + } + } + } + } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java index e9a9f007197a..b426263b9604 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java @@ -231,6 +231,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn keyName); OmKeyInfo dbKeyInfo = omMetadataManager.getKeyTable(getBucketLayout()) .getIfExist(dbKeyName); + validateOptimisticLockingOverwrite(dbKeyInfo, keyArgs); OmBucketInfo bucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName); @@ -440,4 +441,22 @@ public static OMRequest blockCreateKeyWithBucketLayoutFromOldClient( } return req; } + + private void validateOptimisticLockingOverwrite(OmKeyInfo dbKeyInfo, KeyArgs keyArgs) + throws OMException { + if (keyArgs.hasOverwriteObjectID() || keyArgs.hasOverwriteUpdateID()) { + // If a key does not exist, or if it exists but the objectID or updateID + // do not match, then fail this request. + if (dbKeyInfo == null) { + throw new OMException("Key not found during expected overwrite", OMException.ResultCodes.KEY_NOT_FOUND); + } + if (dbKeyInfo.getObjectID() != keyArgs.getOverwriteObjectID()) { + throw new OMException("ObjectID mismatch during expected overwrite", + OMException.ResultCodes.KEY_NOT_FOUND); + } + if (dbKeyInfo.getUpdateID() != keyArgs.getOverwriteUpdateID()) { + throw new OMException("UpdateID mismatch during expected overwrite", OMException.ResultCodes.KEY_NOT_FOUND); + } + } + } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java index a25c4a5bbde0..076df9def713 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java @@ -779,6 +779,16 @@ protected OmKeyInfo prepareFileInfo( dbKeyInfo.getMetadata().clear(); dbKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( keyArgs.getMetadataList())); + if (keyArgs.hasOverwriteObjectID()) { + LOG.info("++ Storing overwrite objectID. Key: {} OverwriteObjectID: {}", + keyArgs.getKeyName(), keyArgs.getOverwriteObjectID()); + dbKeyInfo.setOverwriteObjectID(keyArgs.getOverwriteObjectID()); + } + if (keyArgs.hasOverwriteUpdateID()) { + LOG.info("++ Storing overwrite updateID. Key: {} OverwriteUpdateID: {}", + keyArgs.getKeyName(), keyArgs.getOverwriteUpdateID()); + dbKeyInfo.setOverwriteUpdateID(keyArgs.getOverwriteUpdateID()); + } return dbKeyInfo; } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java index 9719865db196..0d9247aba723 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java @@ -36,6 +36,7 @@ import org.apache.hadoop.ozone.om.exceptions.OMException; import org.apache.hadoop.ozone.om.helpers.BucketLayout; import org.apache.hadoop.ozone.om.helpers.OmBucketInfo; +import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfoGroup; import org.apache.hadoop.ozone.om.helpers.OzoneFSUtils; import org.apache.hadoop.ozone.om.helpers.RepeatedOmKeyInfo; import org.apache.hadoop.ozone.om.request.OMRequestTestUtils; @@ -58,6 +59,8 @@ import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos .OMRequest; +import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.KEY_NOT_FOUND; +import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.OK; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -74,6 +77,8 @@ public class TestOMKeyCommitRequest extends TestOMKeyRequest { private String parentDir; + private Long overwriteObjectID = null; + private Long overwriteUpdateID = null; @Test public void testPreExecute() throws Exception { @@ -116,7 +121,7 @@ public void testValidateAndUpdateCacheWithUnknownBlockId() throws Exception { OMClientResponse omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); - assertEquals(OzoneManagerProtocolProtos.Status.OK, + assertEquals(OK, omClientResponse.getOMResponse().getStatus()); // Entry should be deleted from openKey Table. @@ -183,7 +188,7 @@ public void testValidateAndUpdateCache() throws Exception { OMClientResponse omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); - assertEquals(OzoneManagerProtocolProtos.Status.OK, + assertEquals(OK, omClientResponse.getOMResponse().getStatus()); // Entry should be deleted from openKey Table. @@ -218,6 +223,69 @@ public void testValidateAndUpdateCache() throws Exception { omKeyInfo.getLatestVersionLocations().getLocationList()); } + @Test + public void testCommitWithOptimisticLocking() throws Exception { + Table openKeyTable = omMetadataManager.getOpenKeyTable(BucketLayout.DEFAULT); + Table closedKeyTable = omMetadataManager.getKeyTable(BucketLayout.DEFAULT); + + OMRequest modifiedOmRequest = doPreExecute(createCommitKeyRequest()); + OMKeyCommitRequest omKeyCommitRequest = getOmKeyCommitRequest(modifiedOmRequest); + KeyArgs keyArgs = modifiedOmRequest.getCommitKeyRequest().getKeyArgs(); + + OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName, + omMetadataManager, omKeyCommitRequest.getBucketLayout()); + + // Append new blocks + List allocatedLocationList = + keyArgs.getKeyLocationsList().stream() + .map(OmKeyLocationInfo::getFromProtobuf) + .collect(Collectors.toList()); + + OmKeyInfo.Builder omKeyInfoBuilder = OMRequestTestUtils.createOmKeyInfo( + volumeName, bucketName, keyName, replicationConfig, new OmKeyLocationInfoGroup(version, new ArrayList<>())); + omKeyInfoBuilder.setOverwriteUpdateID(1L); + omKeyInfoBuilder.setOverwriteObjectID(1L); + OmKeyInfo omKeyInfo = omKeyInfoBuilder.build(); + omKeyInfo.appendNewBlocks(allocatedLocationList, false); + + String openKey = getOzonePathKey() + "/" + modifiedOmRequest.getCommitKeyRequest().getClientID(); + + openKeyTable.put(openKey, omKeyInfo); + OmKeyInfo openKeyInfo = openKeyTable.get(openKey); + assertNotNull(openKeyInfo); + // At this stage, we have an openKey, with overwrite object / update ID of 1. + // However there is no closed key entry, so the commit should fail. + OMClientResponse omClientResponse = + omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); + assertEquals(KEY_NOT_FOUND, omClientResponse.getOMResponse().getStatus()); + + // Now add the key to the key table, and try again, but with different object_ID / update_ID + omKeyInfoBuilder.setOverwriteObjectID(null); + omKeyInfoBuilder.setOverwriteUpdateID(null); + omKeyInfoBuilder.setObjectID(0L); + omKeyInfoBuilder.setUpdateID(0L); + OmKeyInfo invalidKeyInfo = omKeyInfoBuilder.build(); + closedKeyTable.put(getOzonePathKey(), invalidKeyInfo); + // This should fail as the IDs are zero and the open key has overwrite IDs of 1. + omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); + assertEquals(KEY_NOT_FOUND, omClientResponse.getOMResponse().getStatus()); + + omKeyInfoBuilder.setObjectID(1L); + omKeyInfoBuilder.setUpdateID(1L); + OmKeyInfo closedKeyInfo = omKeyInfoBuilder.build(); + + closedKeyTable.delete(getOzonePathKey()); + closedKeyTable.put(getOzonePathKey(), closedKeyInfo); + + // Now the key should commit as the IDs match. + omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); + assertEquals(OK, omClientResponse.getOMResponse().getStatus()); + + OmKeyInfo committedKey = closedKeyTable.get(getOzonePathKey()); + assertNull(committedKey.getOverwriteObjectID()); + assertNull(committedKey.getOverwriteUpdateID()); + } + @Test public void testValidateAndUpdateCacheWithUncommittedBlocks() throws Exception { @@ -260,7 +328,7 @@ public void testValidateAndUpdateCacheWithUncommittedBlocks() OMClientResponse omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); - assertEquals(OzoneManagerProtocolProtos.Status.OK, + assertEquals(OK, omClientResponse.getOMResponse().getStatus()); Map toDeleteKeyList @@ -385,7 +453,7 @@ private Map doKeyCommit(boolean isHSync, OMClientResponse omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); - assertEquals(OzoneManagerProtocolProtos.Status.OK, + assertEquals(OK, omClientResponse.getOMResponse().getStatus()); // Key should be present in both OpenKeyTable and KeyTable with HSync commit @@ -550,7 +618,7 @@ public void testValidateAndUpdateCacheWithKeyNotFound() throws Exception { OMClientResponse omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); - assertEquals(OzoneManagerProtocolProtos.Status.KEY_NOT_FOUND, + assertEquals(KEY_NOT_FOUND, omClientResponse.getOMResponse().getStatus()); omKeyInfo = @@ -596,7 +664,7 @@ public void testValidateAndUpdateCacheOnOverwrite() throws Exception { OMClientResponse omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 102L); - assertEquals(OzoneManagerProtocolProtos.Status.OK, omClientResponse.getOMResponse().getStatus()); + assertEquals(OK, omClientResponse.getOMResponse().getStatus()); // New entry should be created in key Table. omKeyInfo = omMetadataManager.getKeyTable(omKeyCommitRequest.getBucketLayout()).get(ozoneKey); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java index 0790e2af3b67..aff2cc509c83 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java @@ -70,6 +70,7 @@ import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ENABLE_FILESYSTEM_PATHS; import static org.apache.hadoop.ozone.om.request.OMRequestTestUtils.addVolumeAndBucketToDB; import static org.apache.hadoop.ozone.om.request.OMRequestTestUtils.createOmKeyInfo; +import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.KEY_NOT_FOUND; import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.NOT_A_FILE; import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.OK; import static org.assertj.core.api.Assertions.assertThat; @@ -120,9 +121,9 @@ private void preExecuteTest(boolean isMultipartKey, int partNumber, long scmBlockSize = ozoneManager.getScmBlockSize(); for (int i = 0; i <= repConfig.getRequiredNodes(); i++) { doPreExecute(createKeyRequest(isMultipartKey, partNumber, - scmBlockSize * i, repConfig)); + scmBlockSize * i, repConfig, null, null)); doPreExecute(createKeyRequest(isMultipartKey, partNumber, - scmBlockSize * i + 1, repConfig)); + scmBlockSize * i + 1, repConfig, null, null)); } } @@ -701,7 +702,7 @@ protected OMRequest createKeyRequest(boolean isMultipartKey, int partNumber, private OMRequest createKeyRequest( boolean isMultipartKey, int partNumber, long keyLength, - ReplicationConfig repConfig) { + ReplicationConfig repConfig, Long overwriteObjectID, Long overwriteUpdateID) { KeyArgs.Builder keyArgs = KeyArgs.newBuilder() .setVolumeName(volumeName).setBucketName(bucketName) @@ -720,6 +721,12 @@ private OMRequest createKeyRequest( if (isMultipartKey) { keyArgs.setMultipartNumber(partNumber); } + if (overwriteObjectID != null) { + keyArgs.setOverwriteObjectID(overwriteObjectID); + } + if (overwriteUpdateID != null) { + keyArgs.setOverwriteUpdateID(overwriteUpdateID); + } OzoneManagerProtocolProtos.CreateKeyRequest createKeyRequest = CreateKeyRequest.newBuilder().setKeyArgs(keyArgs).build(); @@ -903,6 +910,63 @@ public void testKeyCreateInheritParentDefaultAcls( } + @ParameterizedTest + @MethodSource("data") + public void testOptimisticOverwrite( + boolean setKeyPathLock, boolean setFileSystemPaths) throws Exception { + when(ozoneManager.getOzoneLockProvider()).thenReturn( + new OzoneLockProvider(setKeyPathLock, setFileSystemPaths)); + + OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, omMetadataManager, + OmBucketInfo.newBuilder().setVolumeName(volumeName) + .setBucketName(bucketName) + .setBucketLayout(getBucketLayout())); + + // First, create a key with the overwrite IDs - this should fail as no key exists + OMRequest omRequest = createKeyRequest(false, 0, 100, + RatisReplicationConfig.getInstance(THREE), 1L, 1L); + omRequest = doPreExecute(omRequest); + OMKeyCreateRequest omKeyCreateRequest = getOMKeyCreateRequest(omRequest); + OMClientResponse response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 100L); + assertEquals(KEY_NOT_FOUND, response.getOMResponse().getStatus()); + + // Now pre-create the key in the system so we can overwrite it. + createAndCheck(keyName); + // Commit openKey entry. + OMRequestTestUtils.addKeyToTable(false, volumeName, bucketName, + keyName, 0L, RatisReplicationConfig.getInstance(THREE), omMetadataManager); + // Retrieve the committed key info + String ozoneKey = omMetadataManager.getOzoneKey(volumeName, bucketName, keyName); + OmKeyInfo existingKeyInfo = omMetadataManager.getKeyTable(getBucketLayout()).get(ozoneKey); + + // Create a request with object and update IDs which don't match the current key + omRequest = createKeyRequest(false, 0, 100, + RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getObjectID() + 1, + existingKeyInfo.getUpdateID() + 1); + omRequest = doPreExecute(omRequest); + omKeyCreateRequest = getOMKeyCreateRequest(omRequest); + response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 100L); + // Still fails, as the matching key is not present. + assertEquals(KEY_NOT_FOUND, response.getOMResponse().getStatus()); + + // Now create the key with the correct overwrite IDs + omRequest = createKeyRequest(false, 0, 100, + RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getObjectID(), + existingKeyInfo.getUpdateID()); + omRequest = doPreExecute(omRequest); + omKeyCreateRequest = getOMKeyCreateRequest(omRequest); + response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 100L); + assertEquals(OK, response.getOMResponse().getStatus()); + + // Ensure the update / object IDs are persisted in the open key table + String openKey = omMetadataManager.getOpenKey(volumeName, bucketName, + keyName, omRequest.getCreateKeyRequest().getClientID()); + OmKeyInfo openKeyInfo = omMetadataManager.getOpenKeyTable(omKeyCreateRequest.getBucketLayout()).get(openKey); + + assertEquals(existingKeyInfo.getObjectID(), openKeyInfo.getOverwriteObjectID()); + assertEquals(existingKeyInfo.getUpdateID(), openKeyInfo.getOverwriteUpdateID()); + } + /** * Leaf file has ACCESS scope acls which inherited * from parent DEFAULT acls. From 9799ba5feba52eaa21d12124abb62a8d0fe97c5d Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Thu, 14 Mar 2024 14:45:23 +0000 Subject: [PATCH 03/26] Push objectid / updateid through various classes --- .../hadoop/ozone/client/OzoneBucket.java | 6 ++-- .../apache/hadoop/ozone/client/OzoneKey.java | 28 ++++++++++++++++--- .../hadoop/ozone/client/OzoneKeyDetails.java | 4 +-- .../hadoop/ozone/client/rpc/RpcClient.java | 11 ++++++-- .../hadoop/ozone/client/OzoneBucketStub.java | 12 ++++---- 5 files changed, 43 insertions(+), 18 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index 3b76daeba4e5..15ec9aa34af1 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -1287,7 +1287,7 @@ protected List buildKeysWithKeyPrefix( keyInfo.getBucketName(), keyName, keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), - keyInfo.getReplicationConfig(), keyInfo.isFile()); + keyInfo.getReplicationConfig(), keyInfo.isFile(), null, null); }) .filter(key -> StringUtils.startsWith(key.getName(), getKeyPrefix())) .collect(Collectors.toList()); @@ -1675,7 +1675,7 @@ private boolean getChildrenKeys(String keyPrefix, String startKey, keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), keyInfo.getReplicationConfig(), - keyInfo.isFile()); + keyInfo.isFile(), null, null); keysResultList.add(ozoneKey); @@ -1779,7 +1779,7 @@ private void addKeyPrefixInfoToResultList(String keyPrefix, keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), keyInfo.getReplicationConfig(), - keyInfo.isFile()); + keyInfo.isFile(), keyInfo.getObjectID(), keyInfo.getUpdateID()); keysResultList.add(ozoneKey); } } diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java index fba9826df1a1..28f8a43d0d46 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java @@ -70,11 +70,19 @@ public class OzoneKey { * Constructs OzoneKey from OmKeyInfo. * */ + + /** + * The object and update ID of an existing key. These will be null + * if the key is not yet created on OM and read from OM. + */ + private final Long objectID; + private final Long updateID; + @SuppressWarnings("parameternumber") public OzoneKey(String volumeName, String bucketName, String keyName, long size, long creationTime, long modificationTime, ReplicationConfig replicationConfig, - boolean isFile) { + boolean isFile, Long objectID, Long updateID) { this.volumeName = volumeName; this.bucketName = bucketName; this.name = keyName; @@ -83,15 +91,17 @@ public OzoneKey(String volumeName, String bucketName, this.modificationTime = Instant.ofEpochMilli(modificationTime); this.replicationConfig = replicationConfig; this.isFile = isFile; + this.objectID = objectID; + this.updateID = updateID; } @SuppressWarnings("parameternumber") public OzoneKey(String volumeName, String bucketName, String keyName, long size, long creationTime, long modificationTime, ReplicationConfig replicationConfig, - Map metadata, boolean isFile) { + Map metadata, boolean isFile, Long objectID, Long updateID) { this(volumeName, bucketName, keyName, size, creationTime, - modificationTime, replicationConfig, isFile); + modificationTime, replicationConfig, isFile, objectID, updateID); this.metadata.putAll(metadata); } @@ -179,6 +189,16 @@ public ReplicationConfig getReplicationConfig() { return replicationConfig; } + @JsonIgnore + public Long getObjectID() { + return objectID; + } + + @JsonIgnore + public Long getUpdateID() { + return updateID; + } + /** * Returns indicator if key is a file. * @return file @@ -191,7 +211,7 @@ public static OzoneKey fromKeyInfo(OmKeyInfo keyInfo) { return new OzoneKey(keyInfo.getVolumeName(), keyInfo.getBucketName(), keyInfo.getKeyName(), keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), keyInfo.getReplicationConfig(), - keyInfo.getMetadata(), keyInfo.isFile()); + keyInfo.getMetadata(), keyInfo.isFile(), keyInfo.getObjectID(), keyInfo.getUpdateID()); } } diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java index 8c29b66fd34a..bde104b597ae 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java @@ -53,9 +53,9 @@ public OzoneKeyDetails(String volumeName, String bucketName, String keyName, Map metadata, FileEncryptionInfo feInfo, CheckedSupplier contentSupplier, - boolean isFile) { + boolean isFile, Long objectID, Long updateID) { super(volumeName, bucketName, keyName, size, creationTime, - modificationTime, replicationConfig, metadata, isFile); + modificationTime, replicationConfig, metadata, isFile, objectID, updateID); this.ozoneKeyLocations = ozoneKeyLocations; this.feInfo = feInfo; this.contentSupplier = contentSupplier; diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 178a9919c114..10bdc6184aee 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1643,7 +1643,9 @@ public List listKeys(String volumeName, String bucketName, key.getCreationTime(), key.getModificationTime(), key.getReplicationConfig(), - key.isFile())) + key.isFile(), + null, + null)) .collect(Collectors.toList()); } else { List keys = ozoneManagerClient.listKeys( @@ -1656,7 +1658,9 @@ public List listKeys(String volumeName, String bucketName, key.getCreationTime(), key.getModificationTime(), key.getReplicationConfig(), - key.isFile())) + key.isFile(), + key.getObjectID(), + key.getUpdateID())) .collect(Collectors.toList()); } } @@ -1707,7 +1711,8 @@ private OzoneKeyDetails getOzoneKeyDetails(OmKeyInfo keyInfo) { keyInfo.getModificationTime(), ozoneKeyLocations, keyInfo.getReplicationConfig(), keyInfo.getMetadata(), keyInfo.getFileEncryptionInfo(), - () -> getInputStreamWithRetryFunction(keyInfo), keyInfo.isFile()); + () -> getInputStreamWithRetryFunction(keyInfo), keyInfo.isFile(), + keyInfo.getObjectID(), keyInfo.getUpdateID()); } @Override diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java index 0cbe0781c4ba..81113758d06e 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java @@ -162,7 +162,7 @@ public void close() throws IOException { System.currentTimeMillis(), System.currentTimeMillis(), new ArrayList<>(), replicationConfig, metadata, null, - () -> readKey(key), true + () -> readKey(key), true, null, null )); super.close(); } @@ -194,7 +194,7 @@ public void close() throws IOException { System.currentTimeMillis(), System.currentTimeMillis(), new ArrayList<>(), finalReplicationCon, metadata, null, - () -> readKey(key), true + () -> readKey(key), true, null, null )); super.close(); } @@ -231,7 +231,7 @@ public void close() throws IOException { System.currentTimeMillis(), System.currentTimeMillis(), new ArrayList<>(), rConfig, objectMetadata, null, - null, false + null, false, null, null )); } @@ -322,7 +322,7 @@ public OzoneKey headObject(String key) throws IOException { ozoneKeyDetails.getCreationTime().toEpochMilli(), ozoneKeyDetails.getModificationTime().toEpochMilli(), ozoneKeyDetails.getReplicationConfig(), - ozoneKeyDetails.isFile()); + ozoneKeyDetails.isFile(), ozoneKeyDetails.getObjectID(), ozoneKeyDetails.getUpdateID()); } else { throw new OMException(ResultCodes.KEY_NOT_FOUND); } @@ -377,7 +377,7 @@ public Iterator listKeys(String keyPrefix, key.getDataSize(), key.getCreationTime().getEpochSecond() * 1000, key.getModificationTime().getEpochSecond() * 1000, - key.getReplicationConfig(), key.isFile()); + key.getReplicationConfig(), key.isFile(), key.getObjectID(), key.getUpdateID()); }).collect(Collectors.toList()); if (prevKey != null) { @@ -619,7 +619,7 @@ public void createDirectory(String keyName) throws IOException { System.currentTimeMillis(), System.currentTimeMillis(), new ArrayList<>(), replicationConfig, new HashMap<>(), null, - () -> readKey(keyName), false)); + () -> readKey(keyName), false, null, null)); } /** From 5040d2bd8f9210eadab8931fde999182fa0e07c2 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Fri, 15 Mar 2024 10:17:22 +0000 Subject: [PATCH 04/26] Adapt client to be able to overwrite a key --- .../hadoop/ozone/client/OzoneBucket.java | 19 +++++ .../ozone/client/protocol/ClientProtocol.java | 13 +++ .../hadoop/ozone/client/rpc/RpcClient.java | 85 ++++++++++++++----- .../hadoop/ozone/om/helpers/OmKeyArgs.java | 18 ++-- ...ManagerProtocolClientSideTranslatorPB.java | 7 ++ .../rpc/TestOzoneRpcClientAbstract.java | 48 +++++++++++ .../ozone/client/ClientProtocolStub.java | 7 ++ .../hadoop/ozone/client/OzoneBucketStub.java | 32 +++++++ 8 files changed, 203 insertions(+), 26 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index 15ec9aa34af1..b247d58f6cc3 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -469,6 +469,25 @@ public OzoneOutputStream createKey(String key, long size, .createKey(volumeName, name, key, size, replicationConfig, keyMetadata); } + /** + * Overwrite an existing key using optimistic locking. The existingKey must exist in Ozone to allow + * the new key to be created with the same name. Additionally, the existingKey must not have been + * modified since the time its details were read. This is controlled by the objectID and updateID + * fields in the existingKey. If the key is replaced the objectID will change. If it has been updated + * (eg appended) the updateID will change. If either of these have changed since the existingKey + * was read, either the initial key create will fail, or the key will fail to commit after the data + * has been written. + * + * @param existingKey Name of the key to be created. + * @param replicationConfig Replication configuration. + * @return OzoneOutputStream to which the data has to be written. + * @throws IOException + */ + public OzoneOutputStream overWriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig) + throws IOException { + return proxy.overWriteKey(existingKey, replicationConfig); + } + /** * Creates a new key in the bucket, with default replication type RATIS and * with replication factor THREE. diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java index 492cd31b6722..f069bb69501a 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java @@ -353,6 +353,19 @@ OzoneOutputStream createKey(String volumeName, String bucketName, Map metadata) throws IOException; + /** + * Overwrite an existing key using optimistic locking. The OzoneKeyDetails passed must contain + * the objectID and updateID of the key to be overwritten. The existing key must also exist on + * OM and have the same ObjectID and UpdateID as the one passed in or the key create / commit + * will fail. + * @param keyToOverwrite Existing key to overwrite + * @param replicationConfig The replication configuration for the new key + * @return {@link OzoneOutputStream} + * @throws IOException + */ + OzoneOutputStream overWriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig) + throws IOException; + /** * Writes a key in an existing bucket. * @param volumeName Name of the Volume diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 10bdc6184aee..3eb5fdc072df 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1378,26 +1378,7 @@ public OzoneOutputStream createKey( ReplicationConfig replicationConfig, Map metadata) throws IOException { - verifyVolumeName(volumeName); - verifyBucketName(bucketName); - if (checkKeyNameEnabled) { - HddsClientUtils.verifyKeyName(keyName); - } - HddsClientUtils.checkNotNull(keyName); - if (omVersion - .compareTo(OzoneManagerVersion.ERASURE_CODED_STORAGE_SUPPORT) < 0) { - if (replicationConfig != null && - replicationConfig.getReplicationType() - == HddsProtos.ReplicationType.EC) { - throw new IOException("Can not set the replication of the key to" - + " Erasure Coded replication, as OzoneManager does not support" - + " Erasure Coded replication."); - } - } - - if (replicationConfig != null) { - replicationConfigValidator.validate(replicationConfig); - } + createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig); OmKeyArgs.Builder builder = new OmKeyArgs.Builder() .setVolumeName(volumeName) @@ -1420,6 +1401,70 @@ public OzoneOutputStream createKey( return createOutputStream(openKey); } + @Override + public OzoneOutputStream overWriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig) + throws IOException { + if (keyToOverwrite == null) { + throw new IllegalArgumentException("KeyToOverwrite cannot be null"); + } + if (keyToOverwrite.getObjectID() == null) { + throw new IllegalArgumentException("KeyToOverwrite ObjectID cannot be null"); + } + if (keyToOverwrite.getUpdateID() == null) { + throw new IllegalArgumentException("KeyToOverwrite UpdateID cannot be null"); + } + createKeyPreChecks(keyToOverwrite.getVolumeName(), keyToOverwrite.getBucketName(), + keyToOverwrite.getName(), replicationConfig); + + OmKeyArgs.Builder builder = new OmKeyArgs.Builder() + .setVolumeName(keyToOverwrite.getVolumeName()) + .setBucketName(keyToOverwrite.getBucketName()) + .setKeyName(keyToOverwrite.getName()) + .setDataSize(keyToOverwrite.getDataSize()) + .setReplicationConfig(replicationConfig) + .addAllMetadataGdpr(keyToOverwrite.getMetadata()) + // TODO - if we are effectively cloning a key, probably the ACLs should + // be copied over server side. I am not too sure how this works. + .setAcls(getAclList()) + .setLatestVersionLocation(getLatestVersionLocation) + .setOverwriteObjectID(keyToOverwrite.getObjectID()) + .setOverwriteUpdateID(keyToOverwrite.getUpdateID()); + + OpenKeySession openKey = ozoneManagerClient.openKey(builder.build()); + // For bucket with layout OBJECT_STORE, when create an empty file (size=0), + // OM will set DataSize to OzoneConfigKeys#OZONE_SCM_BLOCK_SIZE, + // which will cause S3G's atomic write length check to fail, + // so reset size to 0 here. + if (isS3GRequest.get() && keyToOverwrite.getDataSize() == 0) { + openKey.getKeyInfo().setDataSize(0); + } + return createOutputStream(openKey); + } + + private void createKeyPreChecks(String volumeName, String bucketName, String keyName, + ReplicationConfig replicationConfig) throws IOException { + verifyVolumeName(volumeName); + verifyBucketName(bucketName); + if (checkKeyNameEnabled) { + HddsClientUtils.verifyKeyName(keyName); + } + HddsClientUtils.checkNotNull(keyName); + if (omVersion + .compareTo(OzoneManagerVersion.ERASURE_CODED_STORAGE_SUPPORT) < 0) { + if (replicationConfig != null && + replicationConfig.getReplicationType() + == HddsProtos.ReplicationType.EC) { + throw new IOException("Can not set the replication of the key to" + + " Erasure Coded replication, as OzoneManager does not support" + + " Erasure Coded replication."); + } + } + + if (replicationConfig != null) { + replicationConfigValidator.validate(replicationConfig); + } + } + @Override public OzoneDataStreamOutput createStreamKey( String volumeName, String bucketName, String keyName, long size, diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java index 7a7b4c8000dd..602ac9760c5c 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java @@ -183,7 +183,7 @@ public void addLocationInfo(OmKeyLocationInfo locationInfo) { } public OmKeyArgs.Builder toBuilder() { - return new OmKeyArgs.Builder() + OmKeyArgs.Builder builder = new OmKeyArgs.Builder() .setVolumeName(volumeName) .setBucketName(bucketName) .setKeyName(keyName) @@ -198,9 +198,15 @@ public OmKeyArgs.Builder toBuilder() { .setHeadOp(headOp) .setLatestVersionLocation(latestVersionLocation) .setAcls(acls) - .setForceUpdateContainerCacheFromSCM(forceUpdateContainerCacheFromSCM) - .setOverwriteUpdateID(overwriteUpdateID) - .setOverwriteObjectID(overwriteObjectID); + .setForceUpdateContainerCacheFromSCM(forceUpdateContainerCacheFromSCM); + + if (overwriteUpdateID != null) { + builder.setOverwriteObjectID(overwriteUpdateID); + } + if (overwriteUpdateID != null) { + builder.setOverwriteUpdateID(overwriteUpdateID); + } + return builder; } @Nonnull @@ -216,10 +222,10 @@ public KeyArgs toProtobuf() { .setForceUpdateContainerCacheFromSCM( isForceUpdateContainerCacheFromSCM()); if (overwriteUpdateID != null) { - toBuilder().setOverwriteObjectID(overwriteUpdateID); + builder.setOverwriteObjectID(overwriteUpdateID); } if (overwriteUpdateID != null) { - toBuilder().setOverwriteUpdateID(overwriteUpdateID); + builder.setOverwriteUpdateID(overwriteUpdateID); } return builder.build(); } diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java index 08fa029833e7..e337b920f5c0 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java @@ -734,6 +734,13 @@ public OpenKeySession openKey(OmKeyArgs args) throws IOException { keyArgs.setSortDatanodes(args.getSortDatanodes()); + if (args.getOverwriteObjectID() != null) { + keyArgs.setOverwriteObjectID(args.getOverwriteObjectID()); + } + if (args.getOverwriteUpdateID() != null) { + keyArgs.setOverwriteUpdateID(args.getOverwriteUpdateID()); + } + req.setKeyArgs(keyArgs.build()); OMRequest omRequest = createOMRequest(Type.CreateKey) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java index b8386869308b..725bbf921835 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java @@ -1034,6 +1034,54 @@ public void testPutKey() throws IOException { } } + @Test + public void testOverwriteKey() throws IOException { + String volumeName = UUID.randomUUID().toString(); + String bucketName = UUID.randomUUID().toString(); + + String value = "sample value"; + String overwriteValue = "overwrite value"; + store.createVolume(volumeName); + OzoneVolume volume = store.getVolume(volumeName); + + // TODO - only works on object store layout for now. + BucketArgs args = BucketArgs.newBuilder() + .setBucketLayout(BucketLayout.OBJECT_STORE) + .build(); + + volume.createBucket(bucketName, args); + OzoneBucket bucket = volume.getBucket(bucketName); + + String keyName = UUID.randomUUID().toString(); + try (OzoneOutputStream out = bucket.createKey(keyName, value.getBytes(UTF_8).length, + RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE), new HashMap<>())) { + out.write(value.getBytes(UTF_8)); + } + OzoneKeyDetails keyDetails = bucket.getKey(keyName); + + try (OzoneOutputStream out = bucket.overWriteKey( + keyDetails, RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE))) { + out.write(overwriteValue.getBytes(UTF_8)); + } + + try (OzoneInputStream is = bucket.readKey(keyName)) { + byte[] fileContent = new byte[overwriteValue.getBytes(UTF_8).length]; + is.read(fileContent); + assertEquals(overwriteValue, new String(fileContent, UTF_8)); + } + + // Delete the key + bucket.deleteKey(keyName); + + // Now try the over write again, and it should fail as the originally read key is no longer there. + assertThrows(IOException.class, () -> { + try (OzoneOutputStream out = bucket.overWriteKey( + keyDetails, RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE))) { + out.write(overwriteValue.getBytes(UTF_8)); + } + }); + } + @Test public void testCheckUsedBytesQuota() throws IOException { String volumeName = UUID.randomUUID().toString(); diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java index d9b834c3186d..c7e0495d491c 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java @@ -226,6 +226,13 @@ public OzoneOutputStream createKey(String volumeName, String bucketName, .createKey(keyName, size, replicationConfig, metadata); } + @Override + public OzoneOutputStream overWriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig) + throws IOException { + return getBucket(existingKey.getVolumeName(), existingKey.getBucketName()) + .overWriteKey(existingKey, replicationConfig); + } + @Override public OzoneInputStream getKey(String volumeName, String bucketName, String keyName) throws IOException { diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java index 81113758d06e..09e875496b85 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java @@ -203,6 +203,38 @@ public void close() throws IOException { return new OzoneOutputStream(byteArrayOutputStream, null); } + @Override + public OzoneOutputStream overWriteKey(OzoneKeyDetails existingKey, ReplicationConfig rConfig) + throws IOException { + final ReplicationConfig repConfig; + if (rConfig == null) { + repConfig = getReplicationConfig(); + } else { + repConfig = rConfig; + } + ReplicationConfig finalReplicationCon = repConfig; + ByteArrayOutputStream byteArrayOutputStream = + new KeyMetadataAwareOutputStream(existingKey.getMetadata()) { + @Override + public void close() throws IOException { + keyContents.put(existingKey.getName(), toByteArray()); + keyDetails.put(existingKey.getName(), new OzoneKeyDetails( + getVolumeName(), + getName(), + existingKey.getName(), + existingKey.getDataSize(), + System.currentTimeMillis(), + System.currentTimeMillis(), + new ArrayList<>(), finalReplicationCon, existingKey.getMetadata(), null, + () -> readKey(existingKey.getName()), true, null, null + )); + super.close(); + } + }; + + return new OzoneOutputStream(byteArrayOutputStream, null); + } + @Override public OzoneDataStreamOutput createStreamKey(String key, long size, ReplicationConfig rConfig, From 6f45e67f4ee703ac8f4f58054245fd13848240d1 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Fri, 15 Mar 2024 10:21:58 +0000 Subject: [PATCH 05/26] Remove debugging log messages --- .../org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java index 076df9def713..e0ad37642355 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java @@ -780,13 +780,9 @@ protected OmKeyInfo prepareFileInfo( dbKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( keyArgs.getMetadataList())); if (keyArgs.hasOverwriteObjectID()) { - LOG.info("++ Storing overwrite objectID. Key: {} OverwriteObjectID: {}", - keyArgs.getKeyName(), keyArgs.getOverwriteObjectID()); dbKeyInfo.setOverwriteObjectID(keyArgs.getOverwriteObjectID()); } if (keyArgs.hasOverwriteUpdateID()) { - LOG.info("++ Storing overwrite updateID. Key: {} OverwriteUpdateID: {}", - keyArgs.getKeyName(), keyArgs.getOverwriteUpdateID()); dbKeyInfo.setOverwriteUpdateID(keyArgs.getOverwriteUpdateID()); } return dbKeyInfo; From 0635134a8b59ac894131a83504960fdaccedae5f Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Fri, 15 Mar 2024 11:34:50 +0000 Subject: [PATCH 06/26] Fix find bugs --- .../hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java index 0d9247aba723..5416b63bd1d2 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java @@ -77,8 +77,6 @@ public class TestOMKeyCommitRequest extends TestOMKeyRequest { private String parentDir; - private Long overwriteObjectID = null; - private Long overwriteUpdateID = null; @Test public void testPreExecute() throws Exception { From c1511227e657f26beb690d88f1cbc1c5c9cb5e9d Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Fri, 15 Mar 2024 16:22:28 +0000 Subject: [PATCH 07/26] Ensure test only runs for non FSO for now --- .../hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java index aff2cc509c83..1c657e219495 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java @@ -917,6 +917,11 @@ public void testOptimisticOverwrite( when(ozoneManager.getOzoneLockProvider()).thenReturn( new OzoneLockProvider(setKeyPathLock, setFileSystemPaths)); + if (getBucketLayout() == BucketLayout.FILE_SYSTEM_OPTIMIZED) { + // TODO: Test is not applicable for FSO layout. + return; + } + OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, omMetadataManager, OmBucketInfo.newBuilder().setVolumeName(volumeName) .setBucketName(bucketName) From 908f9cd696d3b625cd304b5746f26bb92a170653 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Fri, 15 Mar 2024 17:41:24 +0000 Subject: [PATCH 08/26] Fix similar commit test on FSO --- .../hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java index 5416b63bd1d2..33731ba33b1c 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java @@ -223,6 +223,11 @@ public void testValidateAndUpdateCache() throws Exception { @Test public void testCommitWithOptimisticLocking() throws Exception { + if (getBucketLayout() == BucketLayout.FILE_SYSTEM_OPTIMIZED) { + // TODO - does not with in FSO for now + return; + } + Table openKeyTable = omMetadataManager.getOpenKeyTable(BucketLayout.DEFAULT); Table closedKeyTable = omMetadataManager.getKeyTable(BucketLayout.DEFAULT); From 3c9c0e217ddf1d6d56f0d63dc7beb7c4809df06a Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Wed, 20 Mar 2024 10:48:05 +0000 Subject: [PATCH 09/26] Rename overWrite to overwrite --- .../main/java/org/apache/hadoop/ozone/client/OzoneBucket.java | 4 ++-- .../apache/hadoop/ozone/client/protocol/ClientProtocol.java | 2 +- .../java/org/apache/hadoop/ozone/client/rpc/RpcClient.java | 2 +- .../hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java | 4 ++-- .../org/apache/hadoop/ozone/client/ClientProtocolStub.java | 4 ++-- .../java/org/apache/hadoop/ozone/client/OzoneBucketStub.java | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index b247d58f6cc3..7d7d1bbd148a 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -483,9 +483,9 @@ public OzoneOutputStream createKey(String key, long size, * @return OzoneOutputStream to which the data has to be written. * @throws IOException */ - public OzoneOutputStream overWriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig) + public OzoneOutputStream overwriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig) throws IOException { - return proxy.overWriteKey(existingKey, replicationConfig); + return proxy.overwriteKey(existingKey, replicationConfig); } /** diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java index f069bb69501a..81f7f1762daf 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java @@ -363,7 +363,7 @@ OzoneOutputStream createKey(String volumeName, String bucketName, * @return {@link OzoneOutputStream} * @throws IOException */ - OzoneOutputStream overWriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig) + OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig) throws IOException; /** diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 3eb5fdc072df..3b7109f71725 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1402,7 +1402,7 @@ public OzoneOutputStream createKey( } @Override - public OzoneOutputStream overWriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig) + public OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig) throws IOException { if (keyToOverwrite == null) { throw new IllegalArgumentException("KeyToOverwrite cannot be null"); diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java index 725bbf921835..68d05fe44bc8 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java @@ -1059,7 +1059,7 @@ public void testOverwriteKey() throws IOException { } OzoneKeyDetails keyDetails = bucket.getKey(keyName); - try (OzoneOutputStream out = bucket.overWriteKey( + try (OzoneOutputStream out = bucket.overwriteKey( keyDetails, RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE))) { out.write(overwriteValue.getBytes(UTF_8)); } @@ -1075,7 +1075,7 @@ public void testOverwriteKey() throws IOException { // Now try the over write again, and it should fail as the originally read key is no longer there. assertThrows(IOException.class, () -> { - try (OzoneOutputStream out = bucket.overWriteKey( + try (OzoneOutputStream out = bucket.overwriteKey( keyDetails, RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE))) { out.write(overwriteValue.getBytes(UTF_8)); } diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java index c7e0495d491c..edcabfcfe2cb 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java @@ -227,10 +227,10 @@ public OzoneOutputStream createKey(String volumeName, String bucketName, } @Override - public OzoneOutputStream overWriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig) + public OzoneOutputStream overwriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig) throws IOException { return getBucket(existingKey.getVolumeName(), existingKey.getBucketName()) - .overWriteKey(existingKey, replicationConfig); + .overwriteKey(existingKey, replicationConfig); } @Override diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java index 09e875496b85..9072585c4d04 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java @@ -204,7 +204,7 @@ public void close() throws IOException { } @Override - public OzoneOutputStream overWriteKey(OzoneKeyDetails existingKey, ReplicationConfig rConfig) + public OzoneOutputStream overwriteKey(OzoneKeyDetails existingKey, ReplicationConfig rConfig) throws IOException { final ReplicationConfig repConfig; if (rConfig == null) { From 17b5ff18a9d4d4572f908364fdb045a39802bc8f Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Wed, 20 Mar 2024 11:17:54 +0000 Subject: [PATCH 10/26] Overload constructors to avoid passing nulls for the new variables --- .../hadoop/ozone/client/OzoneBucket.java | 4 ++-- .../apache/hadoop/ozone/client/OzoneKey.java | 20 ++++++++++++++++++- .../hadoop/ozone/client/OzoneKeyDetails.java | 17 ++++++++++++++++ .../hadoop/ozone/client/OzoneBucketStub.java | 10 +++++----- 4 files changed, 43 insertions(+), 8 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index 7d7d1bbd148a..854362f27c33 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -1306,7 +1306,7 @@ protected List buildKeysWithKeyPrefix( keyInfo.getBucketName(), keyName, keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), - keyInfo.getReplicationConfig(), keyInfo.isFile(), null, null); + keyInfo.getReplicationConfig(), keyInfo.isFile()); }) .filter(key -> StringUtils.startsWith(key.getName(), getKeyPrefix())) .collect(Collectors.toList()); @@ -1694,7 +1694,7 @@ private boolean getChildrenKeys(String keyPrefix, String startKey, keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), keyInfo.getReplicationConfig(), - keyInfo.isFile(), null, null); + keyInfo.isFile()); keysResultList.add(ozoneKey); diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java index 28f8a43d0d46..a8065435c3dc 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java @@ -95,6 +95,25 @@ public OzoneKey(String volumeName, String bucketName, this.updateID = updateID; } + @SuppressWarnings("parameternumber") + public OzoneKey(String volumeName, String bucketName, + String keyName, long size, long creationTime, + long modificationTime, ReplicationConfig replicationConfig, + boolean isFile) { + this(volumeName, bucketName, keyName, size, creationTime, modificationTime, replicationConfig, + isFile, null, null); + } + + @SuppressWarnings("parameternumber") + public OzoneKey(String volumeName, String bucketName, + String keyName, long size, long creationTime, + long modificationTime, ReplicationConfig replicationConfig, + Map metadata, boolean isFile) { + this(volumeName, bucketName, keyName, size, creationTime, + modificationTime, replicationConfig, isFile, null, null); + this.metadata.putAll(metadata); + } + @SuppressWarnings("parameternumber") public OzoneKey(String volumeName, String bucketName, String keyName, long size, long creationTime, @@ -104,7 +123,6 @@ public OzoneKey(String volumeName, String bucketName, modificationTime, replicationConfig, isFile, objectID, updateID); this.metadata.putAll(metadata); } - /** * Returns Volume Name associated with the Key. * diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java index bde104b597ae..90ec66b4302a 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java @@ -61,6 +61,23 @@ public OzoneKeyDetails(String volumeName, String bucketName, String keyName, this.contentSupplier = contentSupplier; } + /** + * Constructs OzoneKeyDetails from OmKeyInfo. + */ + @SuppressWarnings("parameternumber") + public OzoneKeyDetails(String volumeName, String bucketName, String keyName, + long size, long creationTime, long modificationTime, + List ozoneKeyLocations, + ReplicationConfig replicationConfig, + Map metadata, + FileEncryptionInfo feInfo, + CheckedSupplier contentSupplier, + boolean isFile) { + this(volumeName, bucketName, keyName, size, creationTime, + modificationTime, ozoneKeyLocations, replicationConfig, metadata, feInfo, contentSupplier, + isFile, null, null); + } + /** * Returns the location detail information of the specific Key. */ diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java index 9072585c4d04..e5420fed9f1f 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java @@ -162,7 +162,7 @@ public void close() throws IOException { System.currentTimeMillis(), System.currentTimeMillis(), new ArrayList<>(), replicationConfig, metadata, null, - () -> readKey(key), true, null, null + () -> readKey(key), true )); super.close(); } @@ -194,7 +194,7 @@ public void close() throws IOException { System.currentTimeMillis(), System.currentTimeMillis(), new ArrayList<>(), finalReplicationCon, metadata, null, - () -> readKey(key), true, null, null + () -> readKey(key), true )); super.close(); } @@ -226,7 +226,7 @@ public void close() throws IOException { System.currentTimeMillis(), System.currentTimeMillis(), new ArrayList<>(), finalReplicationCon, existingKey.getMetadata(), null, - () -> readKey(existingKey.getName()), true, null, null + () -> readKey(existingKey.getName()), true )); super.close(); } @@ -263,7 +263,7 @@ public void close() throws IOException { System.currentTimeMillis(), System.currentTimeMillis(), new ArrayList<>(), rConfig, objectMetadata, null, - null, false, null, null + null, false )); } @@ -651,7 +651,7 @@ public void createDirectory(String keyName) throws IOException { System.currentTimeMillis(), System.currentTimeMillis(), new ArrayList<>(), replicationConfig, new HashMap<>(), null, - () -> readKey(keyName), false, null, null)); + () -> readKey(keyName), false)); } /** From 0fe12c51dbdb33c25f7848644e0e51c9e234402f Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Wed, 20 Mar 2024 17:55:28 +0000 Subject: [PATCH 11/26] Avoid setting acl in overwrite request. Enhance test to ensure objectID is unchanged, and metadata is copied over to the open key --- .../hadoop/ozone/client/rpc/RpcClient.java | 3 -- .../ozone/om/request/key/OMKeyRequest.java | 11 +++--- .../request/key/TestOMKeyCommitRequest.java | 5 +++ .../request/key/TestOMKeyCreateRequest.java | 34 +++++++++++++------ .../key/TestOMKeyCreateRequestWithFSO.java | 3 +- 5 files changed, 38 insertions(+), 18 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 3b7109f71725..80e23784105e 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1423,9 +1423,6 @@ public OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, Replicatio .setDataSize(keyToOverwrite.getDataSize()) .setReplicationConfig(replicationConfig) .addAllMetadataGdpr(keyToOverwrite.getMetadata()) - // TODO - if we are effectively cloning a key, probably the ACLs should - // be copied over server side. I am not too sure how this works. - .setAcls(getAclList()) .setLatestVersionLocation(getLatestVersionLocation) .setOverwriteObjectID(keyToOverwrite.getObjectID()) .setOverwriteUpdateID(keyToOverwrite.getUpdateID()); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java index e0ad37642355..89fe5560ddf8 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java @@ -775,10 +775,13 @@ protected OmKeyInfo prepareFileInfo( dbKeyInfo.setReplicationConfig(replicationConfig); // Construct a new metadata map from KeyArgs. - // Clear the old one when the key is overwritten. - dbKeyInfo.getMetadata().clear(); - dbKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( - keyArgs.getMetadataList())); + // Clear the old one when the key is overwritten unless it is a key overwrite + // using optimistic commit. + if (!keyArgs.hasOverwriteUpdateID()) { + dbKeyInfo.getMetadata().clear(); + dbKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( + keyArgs.getMetadataList())); + } if (keyArgs.hasOverwriteObjectID()) { dbKeyInfo.setOverwriteObjectID(keyArgs.getOverwriteObjectID()); } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java index 33731ba33b1c..c795c7affc0b 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java @@ -64,6 +64,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -287,6 +288,10 @@ public void testCommitWithOptimisticLocking() throws Exception { OmKeyInfo committedKey = closedKeyTable.get(getOzonePathKey()); assertNull(committedKey.getOverwriteObjectID()); assertNull(committedKey.getOverwriteUpdateID()); + // The object ID should not change when overwriting. + assertEquals(openKeyInfo.getObjectID(), committedKey.getObjectID()); + // Update ID should be changed + assertNotEquals(closedKeyInfo.getUpdateID(), committedKey.getUpdateID()); } @Test diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java index 1c657e219495..d22eb233c7df 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java @@ -932,14 +932,15 @@ public void testOptimisticOverwrite( RatisReplicationConfig.getInstance(THREE), 1L, 1L); omRequest = doPreExecute(omRequest); OMKeyCreateRequest omKeyCreateRequest = getOMKeyCreateRequest(omRequest); - OMClientResponse response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 100L); + OMClientResponse response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); assertEquals(KEY_NOT_FOUND, response.getOMResponse().getStatus()); // Now pre-create the key in the system so we can overwrite it. - createAndCheck(keyName); + Map metadata = Collections.singletonMap("metakey", "metavalue"); + OmKeyInfo createdKeyInfo = createAndCheck(keyName, metadata); // Commit openKey entry. - OMRequestTestUtils.addKeyToTable(false, volumeName, bucketName, - keyName, 0L, RatisReplicationConfig.getInstance(THREE), omMetadataManager); + OMRequestTestUtils.addKeyToTable(false, false, + createdKeyInfo, 0L, 0L, omMetadataManager); // Retrieve the committed key info String ozoneKey = omMetadataManager.getOzoneKey(volumeName, bucketName, keyName); OmKeyInfo existingKeyInfo = omMetadataManager.getKeyTable(getBucketLayout()).get(ozoneKey); @@ -950,7 +951,7 @@ public void testOptimisticOverwrite( existingKeyInfo.getUpdateID() + 1); omRequest = doPreExecute(omRequest); omKeyCreateRequest = getOMKeyCreateRequest(omRequest); - response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 100L); + response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); // Still fails, as the matching key is not present. assertEquals(KEY_NOT_FOUND, response.getOMResponse().getStatus()); @@ -960,7 +961,7 @@ public void testOptimisticOverwrite( existingKeyInfo.getUpdateID()); omRequest = doPreExecute(omRequest); omKeyCreateRequest = getOMKeyCreateRequest(omRequest); - response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 100L); + response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); assertEquals(OK, response.getOMResponse().getStatus()); // Ensure the update / object IDs are persisted in the open key table @@ -970,6 +971,15 @@ public void testOptimisticOverwrite( assertEquals(existingKeyInfo.getObjectID(), openKeyInfo.getOverwriteObjectID()); assertEquals(existingKeyInfo.getUpdateID(), openKeyInfo.getOverwriteUpdateID()); + // The object ID should not change when overwriting. + assertEquals(existingKeyInfo.getObjectID(), openKeyInfo.getObjectID()); + // Creation time should remain the same on overwrite. + assertEquals(existingKeyInfo.getCreationTime(), openKeyInfo.getCreationTime()); + // Update ID should change + assertNotEquals(existingKeyInfo.getUpdateID(), openKeyInfo.getUpdateID()); + // The metadata should be copied over from the existing key. + assertEquals(metadata, existingKeyInfo.getMetadata()); + assertEquals(metadata, openKeyInfo.getMetadata()); } /** @@ -1028,9 +1038,12 @@ private void checkNotAFile(String keyName) throws Exception { assertEquals(NOT_A_FILE, omClientResponse.getOMResponse().getStatus()); } - private void createAndCheck(String keyName) throws Exception { - OMRequest omRequest = createKeyRequest(false, 0, keyName); + createAndCheck(keyName, Collections.emptyMap()); + } + + private OmKeyInfo createAndCheck(String keyName, Map metadata) throws Exception { + OMRequest omRequest = createKeyRequest(false, 0, keyName, metadata); OMKeyCreateRequest omKeyCreateRequest = getOMKeyCreateRequest(omRequest); @@ -1043,10 +1056,10 @@ private void createAndCheck(String keyName) throws Exception { assertEquals(OK, omClientResponse.getOMResponse().getStatus()); - checkCreatedPaths(omKeyCreateRequest, omRequest, keyName); + return checkCreatedPaths(omKeyCreateRequest, omRequest, keyName); } - protected void checkCreatedPaths( + protected OmKeyInfo checkCreatedPaths( OMKeyCreateRequest omKeyCreateRequest, OMRequest omRequest, String keyName) throws Exception { keyName = omKeyCreateRequest.validateAndNormalizeKey(true, keyName); @@ -1061,6 +1074,7 @@ protected void checkCreatedPaths( omMetadataManager.getOpenKeyTable(omKeyCreateRequest.getBucketLayout()) .get(openKey); assertNotNull(omKeyInfo); + return omKeyInfo; } protected long checkIntermediatePaths(Path keyPath) throws Exception { diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequestWithFSO.java index 2a25a9b09686..87badb281261 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequestWithFSO.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequestWithFSO.java @@ -117,7 +117,7 @@ protected void addToKeyTable(String keyName) throws Exception { } @Override - protected void checkCreatedPaths(OMKeyCreateRequest omKeyCreateRequest, + protected OmKeyInfo checkCreatedPaths(OMKeyCreateRequest omKeyCreateRequest, OMRequest omRequest, String keyName) throws Exception { keyName = omKeyCreateRequest.validateAndNormalizeKey(true, keyName, BucketLayout.FILE_SYSTEM_OPTIMIZED); @@ -139,6 +139,7 @@ protected void checkCreatedPaths(OMKeyCreateRequest omKeyCreateRequest, omMetadataManager.getOpenKeyTable(omKeyCreateRequest.getBucketLayout()) .get(openKey); assertNotNull(omKeyInfo); + return omKeyInfo; } @Override From ae4044cc4e0fde0aa5002210c6284d92cd5bb447 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Thu, 21 Mar 2024 12:05:31 +0000 Subject: [PATCH 12/26] Add overwrite object / update ID to the audit if they are present --- .../main/java/org/apache/hadoop/ozone/OzoneConsts.java | 3 +++ .../apache/hadoop/ozone/om/request/RequestAuditor.java | 8 ++++++++ .../hadoop/ozone/om/request/key/OMKeyCommitRequest.java | 8 ++++++-- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java index f3c08b252b1f..a12f0d030acb 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java @@ -343,6 +343,9 @@ private OzoneConsts() { public static final String BUCKET_LAYOUT = "bucketLayout"; public static final String TENANT = "tenant"; public static final String USER_PREFIX = "userPrefix"; + public static final String OVERWRITE_OBJECT_ID = "overwriteObjectID"; + public static final String OVERWRITE_UPDATE_ID = "overwriteUpdateID"; + // For multi-tenancy public static final String TENANT_ID_USERNAME_DELIMITER = "$"; diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java index c0872db0fd61..4a45ed7e3008 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java @@ -80,6 +80,14 @@ default Map buildKeyArgsAuditMap(KeyArgs keyArgs) { auditMap.put(OzoneConsts.REPLICATION_CONFIG, ECReplicationConfig.toString(keyArgs.getEcReplicationConfig())); } + if (keyArgs.hasOverwriteObjectID()) { + auditMap.put(OzoneConsts.OVERWRITE_OBJECT_ID, + String.valueOf(keyArgs.getOverwriteObjectID())); + } + if (keyArgs.hasOverwriteUpdateID()) { + auditMap.put(OzoneConsts.OVERWRITE_UPDATE_ID, + String.valueOf(keyArgs.getOverwriteUpdateID())); + } return auditMap; } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java index 2a9a4093e95c..6f68f5059067 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java @@ -241,7 +241,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn "entry is not found in the OpenKey table", KEY_NOT_FOUND); } - validateOptimisticLockingOverwrite(keyToDelete, omKeyInfo); + validateOptimisticLockingOverwrite(keyToDelete, omKeyInfo, auditMap); // Optimistic locking validation has passed. Now set the overwrite fields to null so they are // not persisted in the key table. omKeyInfo.setOverwriteUpdateID(null); @@ -505,9 +505,13 @@ public static OMRequest disallowHsync( return req; } - private void validateOptimisticLockingOverwrite(OmKeyInfo existing, OmKeyInfo toCommit) + private void validateOptimisticLockingOverwrite(OmKeyInfo existing, OmKeyInfo toCommit, Map auditMap) throws OMException { if (toCommit.getOverwriteObjectID() != null || toCommit.getOverwriteUpdateID() != null) { + // These values are no passed in the request keyArgs, so add them into the auditMap if they are present + // in the open key entry. + auditMap.put(OzoneConsts.OVERWRITE_OBJECT_ID, String.valueOf(toCommit.getOverwriteObjectID())); + auditMap.put(OzoneConsts.OVERWRITE_OBJECT_ID, String.valueOf(toCommit.getOverwriteUpdateID())); if (existing == null) { throw new OMException("Overwrite with optimistic locking is not allowed for a new key", KEY_NOT_FOUND); } From 7bb59e9a95a6d565ada408f53b466c6bd138c097 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Fri, 22 Mar 2024 10:15:53 +0000 Subject: [PATCH 13/26] Minor review comments --- .../org/apache/hadoop/ozone/client/OzoneBucket.java | 12 +++++++----- .../org/apache/hadoop/ozone/client/OzoneKey.java | 5 +++-- .../apache/hadoop/ozone/client/rpc/RpcClient.java | 4 +--- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index 854362f27c33..603b28d183c6 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -472,11 +472,13 @@ public OzoneOutputStream createKey(String key, long size, /** * Overwrite an existing key using optimistic locking. The existingKey must exist in Ozone to allow * the new key to be created with the same name. Additionally, the existingKey must not have been - * modified since the time its details were read. This is controlled by the objectID and updateID - * fields in the existingKey. If the key is replaced the objectID will change. If it has been updated - * (eg appended) the updateID will change. If either of these have changed since the existingKey - * was read, either the initial key create will fail, or the key will fail to commit after the data - * has been written. + * modified since the time its details were read. This is controlled by the updateID + * field in the existingKey. If the key is replaced or updated the updateID will change. If the + * updateID has changed since the existingKey was read, either the initial key create will fail, + * or the key will fail to commit after the data has been written as the checks are carried out + * both a key open and commit time. + * + * For now this feature only works on Object Store Buckets. FSO support will be added a later. * * @param existingKey Name of the key to be created. * @param replicationConfig Replication configuration. diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java index a8065435c3dc..6f9c58065390 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java @@ -72,8 +72,9 @@ public class OzoneKey { */ /** - * The object and update ID of an existing key. These will be null - * if the key is not yet created on OM and read from OM. + * The update ID of an existing key. This will be null if this OzoneKey + * object has not been created from an existing key read from OM, as OM allocates + * the update ID on commit of the key. */ private final Long objectID; private final Long updateID; diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 80e23784105e..8e7a91506d55 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1685,9 +1685,7 @@ public List listKeys(String volumeName, String bucketName, key.getCreationTime(), key.getModificationTime(), key.getReplicationConfig(), - key.isFile(), - null, - null)) + key.isFile())) .collect(Collectors.toList()); } else { List keys = ozoneManagerClient.listKeys( From 8890ed88112cd1b96bcb85c0bd47effa1a3249a0 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Fri, 22 Mar 2024 11:29:08 +0000 Subject: [PATCH 14/26] Remove overwriteObjectID and base all logic on updateID only --- .../org/apache/hadoop/ozone/OzoneConsts.java | 2 -- .../hadoop/ozone/client/OzoneBucket.java | 10 +++--- .../apache/hadoop/ozone/client/OzoneKey.java | 19 ++++-------- .../hadoop/ozone/client/OzoneKeyDetails.java | 6 ++-- .../ozone/client/protocol/ClientProtocol.java | 8 +++-- .../hadoop/ozone/client/rpc/RpcClient.java | 8 +---- .../hadoop/ozone/om/helpers/OmKeyArgs.java | 24 ++------------ .../hadoop/ozone/om/helpers/OmKeyInfo.java | 31 ++----------------- ...ManagerProtocolClientSideTranslatorPB.java | 3 -- .../ozone/om/helpers/TestOmKeyInfo.java | 2 -- .../src/main/proto/OmClientProtocol.proto | 22 ++++++------- .../ozone/om/request/RequestAuditor.java | 4 --- .../om/request/key/OMKeyCommitRequest.java | 15 +++------ .../om/request/key/OMKeyCreateRequest.java | 9 ++---- .../ozone/om/request/key/OMKeyRequest.java | 3 -- .../request/key/TestOMKeyCommitRequest.java | 13 ++------ .../request/key/TestOMKeyCreateRequest.java | 26 ++++++---------- .../hadoop/ozone/client/OzoneBucketStub.java | 4 +-- 18 files changed, 57 insertions(+), 152 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java index a12f0d030acb..5043cd6483db 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java @@ -343,10 +343,8 @@ private OzoneConsts() { public static final String BUCKET_LAYOUT = "bucketLayout"; public static final String TENANT = "tenant"; public static final String USER_PREFIX = "userPrefix"; - public static final String OVERWRITE_OBJECT_ID = "overwriteObjectID"; public static final String OVERWRITE_UPDATE_ID = "overwriteUpdateID"; - // For multi-tenancy public static final String TENANT_ID_USERNAME_DELIMITER = "$"; public static final String DEFAULT_TENANT_BUCKET_NAMESPACE_POLICY_SUFFIX = diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index 603b28d183c6..15d0ef825f99 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -471,10 +471,10 @@ public OzoneOutputStream createKey(String key, long size, /** * Overwrite an existing key using optimistic locking. The existingKey must exist in Ozone to allow - * the new key to be created with the same name. Additionally, the existingKey must not have been - * modified since the time its details were read. This is controlled by the updateID - * field in the existingKey. If the key is replaced or updated the updateID will change. If the - * updateID has changed since the existingKey was read, either the initial key create will fail, + * the new key to be created with the same name. Additionally, the existing Key must not have been + * modified since the time it's details were read. This is controlled by the updateID + * field in the existing Key. If the key is replaced or updated the updateID will change. If the + * updateID has changed since the existing Key was read, either the initial key create will fail, * or the key will fail to commit after the data has been written as the checks are carried out * both a key open and commit time. * @@ -1800,7 +1800,7 @@ private void addKeyPrefixInfoToResultList(String keyPrefix, keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), keyInfo.getReplicationConfig(), - keyInfo.isFile(), keyInfo.getObjectID(), keyInfo.getUpdateID()); + keyInfo.isFile(), keyInfo.getUpdateID()); keysResultList.add(ozoneKey); } } diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java index 6f9c58065390..5e61114a0674 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java @@ -76,14 +76,13 @@ public class OzoneKey { * object has not been created from an existing key read from OM, as OM allocates * the update ID on commit of the key. */ - private final Long objectID; private final Long updateID; @SuppressWarnings("parameternumber") public OzoneKey(String volumeName, String bucketName, String keyName, long size, long creationTime, long modificationTime, ReplicationConfig replicationConfig, - boolean isFile, Long objectID, Long updateID) { + boolean isFile, Long updateID) { this.volumeName = volumeName; this.bucketName = bucketName; this.name = keyName; @@ -92,7 +91,6 @@ public OzoneKey(String volumeName, String bucketName, this.modificationTime = Instant.ofEpochMilli(modificationTime); this.replicationConfig = replicationConfig; this.isFile = isFile; - this.objectID = objectID; this.updateID = updateID; } @@ -102,7 +100,7 @@ public OzoneKey(String volumeName, String bucketName, long modificationTime, ReplicationConfig replicationConfig, boolean isFile) { this(volumeName, bucketName, keyName, size, creationTime, modificationTime, replicationConfig, - isFile, null, null); + isFile, null); } @SuppressWarnings("parameternumber") @@ -111,7 +109,7 @@ public OzoneKey(String volumeName, String bucketName, long modificationTime, ReplicationConfig replicationConfig, Map metadata, boolean isFile) { this(volumeName, bucketName, keyName, size, creationTime, - modificationTime, replicationConfig, isFile, null, null); + modificationTime, replicationConfig, isFile, null); this.metadata.putAll(metadata); } @@ -119,9 +117,9 @@ public OzoneKey(String volumeName, String bucketName, public OzoneKey(String volumeName, String bucketName, String keyName, long size, long creationTime, long modificationTime, ReplicationConfig replicationConfig, - Map metadata, boolean isFile, Long objectID, Long updateID) { + Map metadata, boolean isFile, Long updateID) { this(volumeName, bucketName, keyName, size, creationTime, - modificationTime, replicationConfig, isFile, objectID, updateID); + modificationTime, replicationConfig, isFile, updateID); this.metadata.putAll(metadata); } /** @@ -208,11 +206,6 @@ public ReplicationConfig getReplicationConfig() { return replicationConfig; } - @JsonIgnore - public Long getObjectID() { - return objectID; - } - @JsonIgnore public Long getUpdateID() { return updateID; @@ -230,7 +223,7 @@ public static OzoneKey fromKeyInfo(OmKeyInfo keyInfo) { return new OzoneKey(keyInfo.getVolumeName(), keyInfo.getBucketName(), keyInfo.getKeyName(), keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), keyInfo.getReplicationConfig(), - keyInfo.getMetadata(), keyInfo.isFile(), keyInfo.getObjectID(), keyInfo.getUpdateID()); + keyInfo.getMetadata(), keyInfo.isFile(), keyInfo.getUpdateID()); } } diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java index 90ec66b4302a..c86da1d778c1 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java @@ -53,9 +53,9 @@ public OzoneKeyDetails(String volumeName, String bucketName, String keyName, Map metadata, FileEncryptionInfo feInfo, CheckedSupplier contentSupplier, - boolean isFile, Long objectID, Long updateID) { + boolean isFile, Long updateID) { super(volumeName, bucketName, keyName, size, creationTime, - modificationTime, replicationConfig, metadata, isFile, objectID, updateID); + modificationTime, replicationConfig, metadata, isFile, updateID); this.ozoneKeyLocations = ozoneKeyLocations; this.feInfo = feInfo; this.contentSupplier = contentSupplier; @@ -75,7 +75,7 @@ public OzoneKeyDetails(String volumeName, String bucketName, String keyName, boolean isFile) { this(volumeName, bucketName, keyName, size, creationTime, modificationTime, ozoneKeyLocations, replicationConfig, metadata, feInfo, contentSupplier, - isFile, null, null); + isFile, null); } /** diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java index 81f7f1762daf..6e8f3237f815 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java @@ -355,9 +355,11 @@ OzoneOutputStream createKey(String volumeName, String bucketName, /** * Overwrite an existing key using optimistic locking. The OzoneKeyDetails passed must contain - * the objectID and updateID of the key to be overwritten. The existing key must also exist on - * OM and have the same ObjectID and UpdateID as the one passed in or the key create / commit - * will fail. + * the updateID of the key to be overwritten. The existing key must also exist on + * OM and have the same UpdateID as the one passed in or the key create / commit will fail. + * + * Currently does not work for FSO buckets. Support will be added later. + * * @param keyToOverwrite Existing key to overwrite * @param replicationConfig The replication configuration for the new key * @return {@link OzoneOutputStream} diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 8e7a91506d55..bc7b99f75b15 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1407,9 +1407,6 @@ public OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, Replicatio if (keyToOverwrite == null) { throw new IllegalArgumentException("KeyToOverwrite cannot be null"); } - if (keyToOverwrite.getObjectID() == null) { - throw new IllegalArgumentException("KeyToOverwrite ObjectID cannot be null"); - } if (keyToOverwrite.getUpdateID() == null) { throw new IllegalArgumentException("KeyToOverwrite UpdateID cannot be null"); } @@ -1424,7 +1421,6 @@ public OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, Replicatio .setReplicationConfig(replicationConfig) .addAllMetadataGdpr(keyToOverwrite.getMetadata()) .setLatestVersionLocation(getLatestVersionLocation) - .setOverwriteObjectID(keyToOverwrite.getObjectID()) .setOverwriteUpdateID(keyToOverwrite.getUpdateID()); OpenKeySession openKey = ozoneManagerClient.openKey(builder.build()); @@ -1699,7 +1695,6 @@ public List listKeys(String volumeName, String bucketName, key.getModificationTime(), key.getReplicationConfig(), key.isFile(), - key.getObjectID(), key.getUpdateID())) .collect(Collectors.toList()); } @@ -1751,8 +1746,7 @@ private OzoneKeyDetails getOzoneKeyDetails(OmKeyInfo keyInfo) { keyInfo.getModificationTime(), ozoneKeyLocations, keyInfo.getReplicationConfig(), keyInfo.getMetadata(), keyInfo.getFileEncryptionInfo(), - () -> getInputStreamWithRetryFunction(keyInfo), keyInfo.isFile(), - keyInfo.getObjectID(), keyInfo.getUpdateID()); + () -> getInputStreamWithRetryFunction(keyInfo), keyInfo.isFile(), keyInfo.getUpdateID()); } @Override diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java index 602ac9760c5c..8a3fc3f70da2 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java @@ -52,12 +52,11 @@ public final class OmKeyArgs implements Auditable { private final boolean recursive; private final boolean headOp; private final boolean forceUpdateContainerCacheFromSCM; - // Overwrite objectID and updateID when used in key creation indicate that a - // key with the same keyName should exist with the given object and update IDs, - // and upon commit, the object and updateID of that existing key should be unchanged. + // Overwrite updateID when used in key creation indicates that a + // key with the same keyName should exist with the given update ID. + // Upon commit, the updateID of that existing key should be unchanged. // This is a form of optimistic locking, to ensure the key is not modified between // the time the key is read and the time the key is written. - private Long overwriteObjectID = null; private Long overwriteUpdateID = null; private OmKeyArgs(Builder b) { @@ -77,7 +76,6 @@ private OmKeyArgs(Builder b) { this.recursive = b.recursive; this.headOp = b.headOp; this.forceUpdateContainerCacheFromSCM = b.forceUpdateContainerCacheFromSCM; - this.overwriteObjectID = b.overwriteObjectID; this.overwriteUpdateID = b.overwriteUpdateID; } @@ -153,10 +151,6 @@ public boolean isForceUpdateContainerCacheFromSCM() { return forceUpdateContainerCacheFromSCM; } - public Long getOverwriteObjectID() { - return overwriteObjectID; - } - public Long getOverwriteUpdateID() { return overwriteUpdateID; } @@ -200,9 +194,6 @@ public OmKeyArgs.Builder toBuilder() { .setAcls(acls) .setForceUpdateContainerCacheFromSCM(forceUpdateContainerCacheFromSCM); - if (overwriteUpdateID != null) { - builder.setOverwriteObjectID(overwriteUpdateID); - } if (overwriteUpdateID != null) { builder.setOverwriteUpdateID(overwriteUpdateID); } @@ -221,9 +212,6 @@ public KeyArgs toProtobuf() { .setHeadOp(isHeadOp()) .setForceUpdateContainerCacheFromSCM( isForceUpdateContainerCacheFromSCM()); - if (overwriteUpdateID != null) { - builder.setOverwriteObjectID(overwriteUpdateID); - } if (overwriteUpdateID != null) { builder.setOverwriteUpdateID(overwriteUpdateID); } @@ -250,7 +238,6 @@ public static class Builder { private boolean recursive; private boolean headOp; private boolean forceUpdateContainerCacheFromSCM; - private Long overwriteObjectID = null; private Long overwriteUpdateID = null; public Builder setVolumeName(String volume) { @@ -346,11 +333,6 @@ public Builder setForceUpdateContainerCacheFromSCM(boolean value) { return this; } - public Builder setOverwriteObjectID(long overwriteObjectID) { - this.overwriteObjectID = overwriteObjectID; - return this; - } - public Builder setOverwriteUpdateID(long overwriteUpdateID) { this.overwriteUpdateID = overwriteUpdateID; return this; diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java index c478e943a8ad..477d7bb43da9 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java @@ -100,12 +100,11 @@ public static Codec getCodec(boolean ignorePipeline) { */ private final List acls; - // Overwrite objectID and updateID when used in key creation indicate that a - // key with the same keyName should exist with the given object and update IDs, - // and upon commit, the object and updateID of that existing key should be unchanged. + // OverwriteUpdateID when used in key creation indicates that a key with the same keyName + // should exist with the given updateID. + // Upon commit, the updateID of that existing key should be unchanged. // This is a form of optimistic locking, to ensure the key is not modified between // the time the key is read and the time the key is written. - private Long overwriteObjectID = null; private Long overwriteUpdateID = null; private OmKeyInfo(Builder b) { @@ -126,7 +125,6 @@ private OmKeyInfo(Builder b) { this.fileChecksum = b.fileChecksum; this.fileName = b.fileName; this.isFile = b.isFile; - this.overwriteObjectID = b.overwriteObjectID; this.overwriteUpdateID = b.overwriteUpdateID; } @@ -170,14 +168,6 @@ public String getFileName() { return fileName; } - public void setOverwriteObjectID(Long overwriteObjectID) { - this.overwriteObjectID = overwriteObjectID; - } - - public Long getOverwriteObjectID() { - return overwriteObjectID; - } - public void setOverwriteUpdateID(Long overwriteUpdateID) { this.overwriteUpdateID = overwriteUpdateID; } @@ -460,7 +450,6 @@ public static class Builder { private FileChecksum fileChecksum; private boolean isFile; - private Long overwriteObjectID = null; private Long overwriteUpdateID = null; public Builder() { @@ -578,11 +567,6 @@ public Builder setFile(boolean isAFile) { return this; } - public Builder setOverwriteObjectID(Long existingObjectID) { - this.overwriteObjectID = existingObjectID; - return this; - } - public Builder setOverwriteUpdateID(Long existingUpdateID) { this.overwriteUpdateID = existingUpdateID; return this; @@ -695,9 +679,6 @@ private KeyInfo getProtobuf(boolean ignorePipeline, String fullKeyName, if (overwriteUpdateID != null) { kb.setOverwriteUpdateID(overwriteUpdateID); } - if (overwriteObjectID != null) { - kb.setOverwriteObjectID(overwriteObjectID); - } return kb.build(); } @@ -744,9 +725,6 @@ public static OmKeyInfo getFromProtobuf(KeyInfo keyInfo) throws IOException { if (keyInfo.hasIsFile()) { builder.setFile(keyInfo.getIsFile()); } - if (keyInfo.hasOverwriteObjectID()) { - builder.setOverwriteObjectID(keyInfo.getOverwriteObjectID()); - } if (keyInfo.hasOverwriteUpdateID()) { builder.setOverwriteUpdateID(keyInfo.getOverwriteUpdateID()); } @@ -856,9 +834,6 @@ public OmKeyInfo copyObject() { if (fileChecksum != null) { builder.setFileChecksum(fileChecksum); } - if (overwriteObjectID != null) { - builder.setOverwriteObjectID(overwriteObjectID); - } if (overwriteUpdateID != null) { builder.setOverwriteUpdateID(overwriteUpdateID); } diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java index e337b920f5c0..357a880abab9 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java @@ -734,9 +734,6 @@ public OpenKeySession openKey(OmKeyArgs args) throws IOException { keyArgs.setSortDatanodes(args.getSortDatanodes()); - if (args.getOverwriteObjectID() != null) { - keyArgs.setOverwriteObjectID(args.getOverwriteObjectID()); - } if (args.getOverwriteUpdateID() != null) { keyArgs.setOverwriteUpdateID(args.getOverwriteUpdateID()); } diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java index 62b1d634f625..76e5eb18551f 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java @@ -67,7 +67,6 @@ public void protobufConversion() throws IOException { assertFalse(key.isHsync()); key.getMetadata().put(OzoneConsts.HSYNC_CLIENT_ID, "clientid"); assertTrue(key.isHsync()); - assertEquals(1234L, key.getOverwriteObjectID()); assertEquals(5678L, key.getOverwriteUpdateID()); } @@ -125,7 +124,6 @@ private OmKeyInfo createOmKeyInfo(ReplicationConfig replicationConfig) { .setReplicationConfig(replicationConfig) .addMetadata("key1", "value1") .addMetadata("key2", "value2") - .setOverwriteObjectID(1234L) .setOverwriteUpdateID(5678L) .build(); } diff --git a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto index 39e29144da0c..c8f799ebb151 100644 --- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto +++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto @@ -1025,13 +1025,12 @@ message KeyArgs { optional hadoop.hdds.ECReplicationConfig ecReplicationConfig = 19; // Force OM to update container cache location from SCL optional bool forceUpdateContainerCacheFromSCM = 20; - // Overwrite objectID and updateID when used in key creation indicate that a - // key with the same keyName should exist with the given object and update IDs, - // and upon commit, the object and updateID of that existing key should be unchanged. + // Overwrite updateID when used in key creation indicates that a + // key with the same keyName should exist with the given updateID. + // Upon commit, the updateID of that existing key should be unchanged. // This is a form of optimistic locking, to ensure the key is not modified between // the time the key is read and the time the key is written. - optional uint64 overwriteObjectID = 21; - optional uint64 overwriteUpdateID = 22; + optional uint64 overwriteUpdateID = 21; } message KeyLocation { @@ -1114,13 +1113,12 @@ message KeyInfo { optional hadoop.hdds.ECReplicationConfig ecReplicationConfig = 17; optional FileChecksumProto fileChecksum = 18; optional bool isFile = 19; - // Overwrite objectID and updateID when used in key creation indicate that a - // key with the same keyName should exist with the given object and update IDs, - // and upon commit, the object and updateID of that existing key should be unchanged. - // This is a form of optimistic locking, to ensure the key is not modified between - // the time the key is read and the time the key is written. - optional uint64 overwriteObjectID = 20; - optional uint64 overwriteUpdateID = 21; + // Overwrite updateID when used in key creation indicates that a + // key with the same keyName should exist with the given updateID. + // Upon commit, the updateID of that existing key should be unchanged. + // This is a form of optimistic locking, to ensure the key is not modified between + // the time the key is read and the time the key is written. + optional uint64 overwriteUpdateID = 20; } message BasicKeyInfo { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java index 4a45ed7e3008..f00a7d2c99eb 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java @@ -80,10 +80,6 @@ default Map buildKeyArgsAuditMap(KeyArgs keyArgs) { auditMap.put(OzoneConsts.REPLICATION_CONFIG, ECReplicationConfig.toString(keyArgs.getEcReplicationConfig())); } - if (keyArgs.hasOverwriteObjectID()) { - auditMap.put(OzoneConsts.OVERWRITE_OBJECT_ID, - String.valueOf(keyArgs.getOverwriteObjectID())); - } if (keyArgs.hasOverwriteUpdateID()) { auditMap.put(OzoneConsts.OVERWRITE_UPDATE_ID, String.valueOf(keyArgs.getOverwriteUpdateID())); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java index 6f68f5059067..ca43a9e050c6 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java @@ -245,7 +245,6 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn // Optimistic locking validation has passed. Now set the overwrite fields to null so they are // not persisted in the key table. omKeyInfo.setOverwriteUpdateID(null); - omKeyInfo.setOverwriteObjectID(null); omKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( commitKeyArgs.getMetadataList())); @@ -507,20 +506,16 @@ public static OMRequest disallowHsync( private void validateOptimisticLockingOverwrite(OmKeyInfo existing, OmKeyInfo toCommit, Map auditMap) throws OMException { - if (toCommit.getOverwriteObjectID() != null || toCommit.getOverwriteUpdateID() != null) { + if (toCommit.getOverwriteUpdateID() != null) { // These values are no passed in the request keyArgs, so add them into the auditMap if they are present // in the open key entry. - auditMap.put(OzoneConsts.OVERWRITE_OBJECT_ID, String.valueOf(toCommit.getOverwriteObjectID())); - auditMap.put(OzoneConsts.OVERWRITE_OBJECT_ID, String.valueOf(toCommit.getOverwriteUpdateID())); + auditMap.put(OzoneConsts.OVERWRITE_UPDATE_ID, String.valueOf(toCommit.getOverwriteUpdateID())); if (existing == null) { throw new OMException("Overwrite with optimistic locking is not allowed for a new key", KEY_NOT_FOUND); } - if (!toCommit.getOverwriteObjectID().equals(existing.getObjectID()) || - !toCommit.getOverwriteUpdateID().equals(existing.getUpdateID())) { - throw new OMException("Cannot commit as current objectID / updateID (" - + existing.getObjectID() + " / " + existing.getUpdateID() + - ") does not match with the overwrite objectID / updateID (" - + toCommit.getOverwriteObjectID() + " / " + toCommit.getOverwriteUpdateID() + ")", KEY_NOT_FOUND); + if (!toCommit.getOverwriteUpdateID().equals(existing.getUpdateID())) { + throw new OMException("Cannot commit as current updateID (" + existing.getUpdateID() + + ") does not match with the overwrite updateID (" + toCommit.getOverwriteUpdateID() + ")", KEY_NOT_FOUND); } } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java index b426263b9604..3cc8d8888249 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java @@ -444,16 +444,11 @@ public static OMRequest blockCreateKeyWithBucketLayoutFromOldClient( private void validateOptimisticLockingOverwrite(OmKeyInfo dbKeyInfo, KeyArgs keyArgs) throws OMException { - if (keyArgs.hasOverwriteObjectID() || keyArgs.hasOverwriteUpdateID()) { - // If a key does not exist, or if it exists but the objectID or updateID - // do not match, then fail this request. + if (keyArgs.hasOverwriteUpdateID()) { + // If a key does not exist, or if it exists but the updateID do not match, then fail this request. if (dbKeyInfo == null) { throw new OMException("Key not found during expected overwrite", OMException.ResultCodes.KEY_NOT_FOUND); } - if (dbKeyInfo.getObjectID() != keyArgs.getOverwriteObjectID()) { - throw new OMException("ObjectID mismatch during expected overwrite", - OMException.ResultCodes.KEY_NOT_FOUND); - } if (dbKeyInfo.getUpdateID() != keyArgs.getOverwriteUpdateID()) { throw new OMException("UpdateID mismatch during expected overwrite", OMException.ResultCodes.KEY_NOT_FOUND); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java index 89fe5560ddf8..dc392cd430eb 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java @@ -782,9 +782,6 @@ protected OmKeyInfo prepareFileInfo( dbKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( keyArgs.getMetadataList())); } - if (keyArgs.hasOverwriteObjectID()) { - dbKeyInfo.setOverwriteObjectID(keyArgs.getOverwriteObjectID()); - } if (keyArgs.hasOverwriteUpdateID()) { dbKeyInfo.setOverwriteUpdateID(keyArgs.getOverwriteUpdateID()); } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java index c795c7affc0b..4a26db9f1e6f 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java @@ -225,7 +225,7 @@ public void testValidateAndUpdateCache() throws Exception { @Test public void testCommitWithOptimisticLocking() throws Exception { if (getBucketLayout() == BucketLayout.FILE_SYSTEM_OPTIMIZED) { - // TODO - does not with in FSO for now + // TODO - does not work with in FSO for now return; } @@ -248,7 +248,6 @@ public void testCommitWithOptimisticLocking() throws Exception { OmKeyInfo.Builder omKeyInfoBuilder = OMRequestTestUtils.createOmKeyInfo( volumeName, bucketName, keyName, replicationConfig, new OmKeyLocationInfoGroup(version, new ArrayList<>())); omKeyInfoBuilder.setOverwriteUpdateID(1L); - omKeyInfoBuilder.setOverwriteObjectID(1L); OmKeyInfo omKeyInfo = omKeyInfoBuilder.build(); omKeyInfo.appendNewBlocks(allocatedLocationList, false); @@ -257,16 +256,14 @@ public void testCommitWithOptimisticLocking() throws Exception { openKeyTable.put(openKey, omKeyInfo); OmKeyInfo openKeyInfo = openKeyTable.get(openKey); assertNotNull(openKeyInfo); - // At this stage, we have an openKey, with overwrite object / update ID of 1. + // At this stage, we have an openKey, with overwrite update ID of 1. // However there is no closed key entry, so the commit should fail. OMClientResponse omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); assertEquals(KEY_NOT_FOUND, omClientResponse.getOMResponse().getStatus()); - // Now add the key to the key table, and try again, but with different object_ID / update_ID - omKeyInfoBuilder.setOverwriteObjectID(null); + // Now add the key to the key table, and try again, but with different update_ID omKeyInfoBuilder.setOverwriteUpdateID(null); - omKeyInfoBuilder.setObjectID(0L); omKeyInfoBuilder.setUpdateID(0L); OmKeyInfo invalidKeyInfo = omKeyInfoBuilder.build(); closedKeyTable.put(getOzonePathKey(), invalidKeyInfo); @@ -274,7 +271,6 @@ public void testCommitWithOptimisticLocking() throws Exception { omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); assertEquals(KEY_NOT_FOUND, omClientResponse.getOMResponse().getStatus()); - omKeyInfoBuilder.setObjectID(1L); omKeyInfoBuilder.setUpdateID(1L); OmKeyInfo closedKeyInfo = omKeyInfoBuilder.build(); @@ -286,10 +282,7 @@ public void testCommitWithOptimisticLocking() throws Exception { assertEquals(OK, omClientResponse.getOMResponse().getStatus()); OmKeyInfo committedKey = closedKeyTable.get(getOzonePathKey()); - assertNull(committedKey.getOverwriteObjectID()); assertNull(committedKey.getOverwriteUpdateID()); - // The object ID should not change when overwriting. - assertEquals(openKeyInfo.getObjectID(), committedKey.getObjectID()); // Update ID should be changed assertNotEquals(closedKeyInfo.getUpdateID(), committedKey.getUpdateID()); } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java index d22eb233c7df..7372f4a4e4e3 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java @@ -121,9 +121,9 @@ private void preExecuteTest(boolean isMultipartKey, int partNumber, long scmBlockSize = ozoneManager.getScmBlockSize(); for (int i = 0; i <= repConfig.getRequiredNodes(); i++) { doPreExecute(createKeyRequest(isMultipartKey, partNumber, - scmBlockSize * i, repConfig, null, null)); + scmBlockSize * i, repConfig, null)); doPreExecute(createKeyRequest(isMultipartKey, partNumber, - scmBlockSize * i + 1, repConfig, null, null)); + scmBlockSize * i + 1, repConfig, null)); } } @@ -702,7 +702,7 @@ protected OMRequest createKeyRequest(boolean isMultipartKey, int partNumber, private OMRequest createKeyRequest( boolean isMultipartKey, int partNumber, long keyLength, - ReplicationConfig repConfig, Long overwriteObjectID, Long overwriteUpdateID) { + ReplicationConfig repConfig, Long overwriteUpdateID) { KeyArgs.Builder keyArgs = KeyArgs.newBuilder() .setVolumeName(volumeName).setBucketName(bucketName) @@ -721,9 +721,6 @@ private OMRequest createKeyRequest( if (isMultipartKey) { keyArgs.setMultipartNumber(partNumber); } - if (overwriteObjectID != null) { - keyArgs.setOverwriteObjectID(overwriteObjectID); - } if (overwriteUpdateID != null) { keyArgs.setOverwriteUpdateID(overwriteUpdateID); } @@ -927,9 +924,9 @@ public void testOptimisticOverwrite( .setBucketName(bucketName) .setBucketLayout(getBucketLayout())); - // First, create a key with the overwrite IDs - this should fail as no key exists + // First, create a key with the overwrite ID - this should fail as no key exists OMRequest omRequest = createKeyRequest(false, 0, 100, - RatisReplicationConfig.getInstance(THREE), 1L, 1L); + RatisReplicationConfig.getInstance(THREE), 1L); omRequest = doPreExecute(omRequest); OMKeyCreateRequest omKeyCreateRequest = getOMKeyCreateRequest(omRequest); OMClientResponse response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); @@ -945,10 +942,9 @@ public void testOptimisticOverwrite( String ozoneKey = omMetadataManager.getOzoneKey(volumeName, bucketName, keyName); OmKeyInfo existingKeyInfo = omMetadataManager.getKeyTable(getBucketLayout()).get(ozoneKey); - // Create a request with object and update IDs which don't match the current key + // Create a request with an update ID which doesn't match the current key omRequest = createKeyRequest(false, 0, 100, - RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getObjectID() + 1, - existingKeyInfo.getUpdateID() + 1); + RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getUpdateID() + 1); omRequest = doPreExecute(omRequest); omKeyCreateRequest = getOMKeyCreateRequest(omRequest); response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); @@ -957,22 +953,18 @@ public void testOptimisticOverwrite( // Now create the key with the correct overwrite IDs omRequest = createKeyRequest(false, 0, 100, - RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getObjectID(), - existingKeyInfo.getUpdateID()); + RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getUpdateID()); omRequest = doPreExecute(omRequest); omKeyCreateRequest = getOMKeyCreateRequest(omRequest); response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); assertEquals(OK, response.getOMResponse().getStatus()); - // Ensure the update / object IDs are persisted in the open key table + // Ensure the updateID is persisted in the open key table String openKey = omMetadataManager.getOpenKey(volumeName, bucketName, keyName, omRequest.getCreateKeyRequest().getClientID()); OmKeyInfo openKeyInfo = omMetadataManager.getOpenKeyTable(omKeyCreateRequest.getBucketLayout()).get(openKey); - assertEquals(existingKeyInfo.getObjectID(), openKeyInfo.getOverwriteObjectID()); assertEquals(existingKeyInfo.getUpdateID(), openKeyInfo.getOverwriteUpdateID()); - // The object ID should not change when overwriting. - assertEquals(existingKeyInfo.getObjectID(), openKeyInfo.getObjectID()); // Creation time should remain the same on overwrite. assertEquals(existingKeyInfo.getCreationTime(), openKeyInfo.getCreationTime()); // Update ID should change diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java index e5420fed9f1f..e2baec7127ed 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java @@ -354,7 +354,7 @@ public OzoneKey headObject(String key) throws IOException { ozoneKeyDetails.getCreationTime().toEpochMilli(), ozoneKeyDetails.getModificationTime().toEpochMilli(), ozoneKeyDetails.getReplicationConfig(), - ozoneKeyDetails.isFile(), ozoneKeyDetails.getObjectID(), ozoneKeyDetails.getUpdateID()); + ozoneKeyDetails.isFile(), ozoneKeyDetails.getUpdateID()); } else { throw new OMException(ResultCodes.KEY_NOT_FOUND); } @@ -409,7 +409,7 @@ public Iterator listKeys(String keyPrefix, key.getDataSize(), key.getCreationTime().getEpochSecond() * 1000, key.getModificationTime().getEpochSecond() * 1000, - key.getReplicationConfig(), key.isFile(), key.getObjectID(), key.getUpdateID()); + key.getReplicationConfig(), key.isFile(), key.getUpdateID()); }).collect(Collectors.toList()); if (prevKey != null) { From a360d457f070cde4b6e465ddacca02187d8ddd26 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Fri, 22 Mar 2024 11:32:41 +0000 Subject: [PATCH 15/26] Block non ObjectStore buckets from using optimistic overwrite for now --- .../main/java/org/apache/hadoop/ozone/client/OzoneBucket.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index 15d0ef825f99..5f526f060e65 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -487,6 +487,9 @@ public OzoneOutputStream createKey(String key, long size, */ public OzoneOutputStream overwriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig) throws IOException { + if (this.bucketLayout != BucketLayout.OBJECT_STORE) { + throw new IllegalArgumentException("Optimistic locking is only supported on Object Store Buckets"); + } return proxy.overwriteKey(existingKey, replicationConfig); } From 3f3a49d3b5ad911548f0cc5765b484156ab19cf3 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Fri, 22 Mar 2024 16:52:39 +0000 Subject: [PATCH 16/26] Validate existing ACLs are copied over from existing to overwrite key --- .../request/key/TestOMKeyCommitRequest.java | 6 ++++ .../request/key/TestOMKeyCreateRequest.java | 30 ++++++++++++------- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java index 4a26db9f1e6f..475f43e32a2f 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.UUID; @@ -31,6 +32,7 @@ import org.apache.hadoop.hdds.utils.db.BatchOperation; import org.apache.hadoop.hdds.utils.db.Table; import org.apache.hadoop.ozone.OmUtils; +import org.apache.hadoop.ozone.OzoneAcl; import org.apache.hadoop.ozone.OzoneConfigKeys; import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.om.exceptions.OMException; @@ -250,12 +252,15 @@ public void testCommitWithOptimisticLocking() throws Exception { omKeyInfoBuilder.setOverwriteUpdateID(1L); OmKeyInfo omKeyInfo = omKeyInfoBuilder.build(); omKeyInfo.appendNewBlocks(allocatedLocationList, false); + List acls = Collections.singletonList(OzoneAcl.parseAcl("user:foo:rw")); + omKeyInfo.addAcl(acls.get(0)); String openKey = getOzonePathKey() + "/" + modifiedOmRequest.getCommitKeyRequest().getClientID(); openKeyTable.put(openKey, omKeyInfo); OmKeyInfo openKeyInfo = openKeyTable.get(openKey); assertNotNull(openKeyInfo); + assertEquals(acls, openKeyInfo.getAcls()); // At this stage, we have an openKey, with overwrite update ID of 1. // However there is no closed key entry, so the commit should fail. OMClientResponse omClientResponse = @@ -285,6 +290,7 @@ public void testCommitWithOptimisticLocking() throws Exception { assertNull(committedKey.getOverwriteUpdateID()); // Update ID should be changed assertNotEquals(closedKeyInfo.getUpdateID(), committedKey.getUpdateID()); + assertEquals(acls, committedKey.getAcls()); } @Test diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java index 7372f4a4e4e3..cc6bc1ecb2b1 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java @@ -483,7 +483,7 @@ public void testOverwritingExistingMetadata( Map initialMetadata = Collections.singletonMap("initialKey", "initialValue"); OMRequest initialRequest = - createKeyRequest(false, 0, keyName, initialMetadata); + createKeyRequest(false, 0, keyName, initialMetadata, Collections.emptyList()); OMKeyCreateRequest initialOmKeyCreateRequest = new OMKeyCreateRequest(initialRequest, getBucketLayout()); OMClientResponse initialResponse = @@ -501,7 +501,7 @@ public void testOverwritingExistingMetadata( Map updatedMetadata = Collections.singletonMap("initialKey", "updatedValue"); OMRequest updatedRequest = - createKeyRequest(false, 0, keyName, updatedMetadata); + createKeyRequest(false, 0, keyName, updatedMetadata, Collections.emptyList()); OMKeyCreateRequest updatedOmKeyCreateRequest = new OMKeyCreateRequest(updatedRequest, getBucketLayout()); @@ -521,7 +521,7 @@ public void testCreationWithoutMetadataFollowedByOverwriteWithMetadata( // Create the key request without any initial metadata OMRequest createRequestWithoutMetadata = createKeyRequest(false, 0, keyName, - null); // Passing 'null' for metadata + null, Collections.emptyList()); // Passing 'null' for metadata OMKeyCreateRequest createOmKeyCreateRequest = new OMKeyCreateRequest(createRequestWithoutMetadata, getBucketLayout()); @@ -544,7 +544,7 @@ public void testCreationWithoutMetadataFollowedByOverwriteWithMetadata( // Overwrite the previously created key with new metadata OMRequest overwriteRequestWithMetadata = - createKeyRequest(false, 0, keyName, overwriteMetadata); + createKeyRequest(false, 0, keyName, overwriteMetadata, Collections.emptyList()); OMKeyCreateRequest overwriteOmKeyCreateRequest = new OMKeyCreateRequest(overwriteRequestWithMetadata, getBucketLayout()); @@ -649,7 +649,7 @@ protected OMRequest createKeyRequest(boolean isMultipartKey, int partNumber) { private OMRequest createKeyRequest(boolean isMultipartKey, int partNumber, String keyName) { - return createKeyRequest(isMultipartKey, partNumber, keyName, null); + return createKeyRequest(isMultipartKey, partNumber, keyName, null, Collections.emptyList()); } /** @@ -666,7 +666,8 @@ private OMRequest createKeyRequest(boolean isMultipartKey, int partNumber, */ protected OMRequest createKeyRequest(boolean isMultipartKey, int partNumber, String keyName, - Map metadata) { + Map metadata, + List acls) { KeyArgs.Builder keyArgs = KeyArgs.newBuilder() .setVolumeName(volumeName) .setBucketName(bucketName) @@ -677,6 +678,9 @@ protected OMRequest createKeyRequest(boolean isMultipartKey, int partNumber, .setType(replicationConfig.getReplicationType()) .setLatestVersionLocation(true); + for (OzoneAcl acl : acls) { + keyArgs.addAcls(OzoneAcl.toProtobuf(acl)); + } // Configure for multipart upload, if applicable if (isMultipartKey) { keyArgs.setDataSize(dataSize).setMultipartNumber(partNumber); @@ -934,13 +938,16 @@ public void testOptimisticOverwrite( // Now pre-create the key in the system so we can overwrite it. Map metadata = Collections.singletonMap("metakey", "metavalue"); - OmKeyInfo createdKeyInfo = createAndCheck(keyName, metadata); + List acls = Collections.singletonList(OzoneAcl.parseAcl("user:foo:rw")); + OmKeyInfo createdKeyInfo = createAndCheck(keyName, metadata, acls); // Commit openKey entry. OMRequestTestUtils.addKeyToTable(false, false, createdKeyInfo, 0L, 0L, omMetadataManager); // Retrieve the committed key info String ozoneKey = omMetadataManager.getOzoneKey(volumeName, bucketName, keyName); OmKeyInfo existingKeyInfo = omMetadataManager.getKeyTable(getBucketLayout()).get(ozoneKey); + List existingAcls = existingKeyInfo.getAcls(); + assertEquals(acls, existingAcls); // Create a request with an update ID which doesn't match the current key omRequest = createKeyRequest(false, 0, 100, @@ -972,6 +979,8 @@ public void testOptimisticOverwrite( // The metadata should be copied over from the existing key. assertEquals(metadata, existingKeyInfo.getMetadata()); assertEquals(metadata, openKeyInfo.getMetadata()); + // Ensure the ACLS are copied over from the existing key. + assertEquals(existingAcls, openKeyInfo.getAcls()); } /** @@ -1031,11 +1040,12 @@ private void checkNotAFile(String keyName) throws Exception { } private void createAndCheck(String keyName) throws Exception { - createAndCheck(keyName, Collections.emptyMap()); + createAndCheck(keyName, Collections.emptyMap(), Collections.emptyList()); } - private OmKeyInfo createAndCheck(String keyName, Map metadata) throws Exception { - OMRequest omRequest = createKeyRequest(false, 0, keyName, metadata); + private OmKeyInfo createAndCheck(String keyName, Map metadata, List acls) + throws Exception { + OMRequest omRequest = createKeyRequest(false, 0, keyName, metadata, acls); OMKeyCreateRequest omKeyCreateRequest = getOMKeyCreateRequest(omRequest); From 525685c317ccd3bd74b4c902cda51400b8976fac Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Tue, 2 Apr 2024 18:09:55 +0100 Subject: [PATCH 17/26] Trigger rebuild From 112e2e18812b961eb2b9d0a375514f19c9c86867 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Tue, 30 Apr 2024 14:00:33 +0100 Subject: [PATCH 18/26] Rename overwriteUpdateID to overwriteGeneration --- .../org/apache/hadoop/ozone/OzoneConsts.java | 2 +- .../hadoop/ozone/client/rpc/RpcClient.java | 4 +- .../hadoop/ozone/om/helpers/OmKeyArgs.java | 33 +++++++-------- .../hadoop/ozone/om/helpers/OmKeyInfo.java | 41 ++++++++++--------- ...ManagerProtocolClientSideTranslatorPB.java | 4 +- .../ozone/om/helpers/TestOmKeyInfo.java | 4 +- .../src/main/proto/OmClientProtocol.proto | 26 ++++++------ .../ozone/om/request/RequestAuditor.java | 6 +-- .../om/request/key/OMKeyCommitRequest.java | 21 +++++----- .../om/request/key/OMKeyCreateRequest.java | 10 ++--- .../ozone/om/request/key/OMKeyRequest.java | 8 ++-- .../request/key/TestOMKeyCommitRequest.java | 14 +++---- .../request/key/TestOMKeyCreateRequest.java | 14 +++---- 13 files changed, 97 insertions(+), 90 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java index 5043cd6483db..6e15804450ad 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java @@ -343,7 +343,7 @@ private OzoneConsts() { public static final String BUCKET_LAYOUT = "bucketLayout"; public static final String TENANT = "tenant"; public static final String USER_PREFIX = "userPrefix"; - public static final String OVERWRITE_UPDATE_ID = "overwriteUpdateID"; + public static final String OVERWRITE_GENERATION = "overwriteGeneration"; // For multi-tenancy public static final String TENANT_ID_USERNAME_DELIMITER = "$"; diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 3b2cb8f0feb9..1721e132ba4f 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1397,6 +1397,7 @@ public OzoneOutputStream createKey( return createOutputStream(openKey); } + // STODO - rename this API and params. @Override public OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig) throws IOException { @@ -1417,7 +1418,8 @@ public OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, Replicatio .setReplicationConfig(replicationConfig) .addAllMetadataGdpr(keyToOverwrite.getMetadata()) .setLatestVersionLocation(getLatestVersionLocation) - .setOverwriteUpdateID(keyToOverwrite.getUpdateID()); + // STODO - keyToOverwrite should have a getGeneration method rather than updateID + .setOverwriteGeneration(keyToOverwrite.getUpdateID()); OpenKeySession openKey = ozoneManagerClient.openKey(builder.build()); // For bucket with layout OBJECT_STORE, when create an empty file (size=0), diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java index 8a3fc3f70da2..e6ba12a51237 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java @@ -52,12 +52,13 @@ public final class OmKeyArgs implements Auditable { private final boolean recursive; private final boolean headOp; private final boolean forceUpdateContainerCacheFromSCM; - // Overwrite updateID when used in key creation indicates that a - // key with the same keyName should exist with the given update ID. - // Upon commit, the updateID of that existing key should be unchanged. - // This is a form of optimistic locking, to ensure the key is not modified between - // the time the key is read and the time the key is written. - private Long overwriteUpdateID = null; + // OverwriteGeneration, when used in key creation indicates that a + // key with the same keyName should exist with the given generation. + // For a key commit to succeed, the original key should still be present with the + // generation unchanged. + // This allows a key to be created an committed atomically if the original has not + // been modified. + private Long overwriteGeneration = null; private OmKeyArgs(Builder b) { this.volumeName = b.volumeName; @@ -76,7 +77,7 @@ private OmKeyArgs(Builder b) { this.recursive = b.recursive; this.headOp = b.headOp; this.forceUpdateContainerCacheFromSCM = b.forceUpdateContainerCacheFromSCM; - this.overwriteUpdateID = b.overwriteUpdateID; + this.overwriteGeneration = b.overwriteGeneration; } public boolean getIsMultipartKey() { @@ -151,8 +152,8 @@ public boolean isForceUpdateContainerCacheFromSCM() { return forceUpdateContainerCacheFromSCM; } - public Long getOverwriteUpdateID() { - return overwriteUpdateID; + public Long getOverwriteGeneration() { + return overwriteGeneration; } @Override @@ -194,8 +195,8 @@ public OmKeyArgs.Builder toBuilder() { .setAcls(acls) .setForceUpdateContainerCacheFromSCM(forceUpdateContainerCacheFromSCM); - if (overwriteUpdateID != null) { - builder.setOverwriteUpdateID(overwriteUpdateID); + if (overwriteGeneration != null) { + builder.setOverwriteGeneration(overwriteGeneration); } return builder; } @@ -212,8 +213,8 @@ public KeyArgs toProtobuf() { .setHeadOp(isHeadOp()) .setForceUpdateContainerCacheFromSCM( isForceUpdateContainerCacheFromSCM()); - if (overwriteUpdateID != null) { - builder.setOverwriteUpdateID(overwriteUpdateID); + if (overwriteGeneration != null) { + builder.setOverwriteGeneration(overwriteGeneration); } return builder.build(); } @@ -238,7 +239,7 @@ public static class Builder { private boolean recursive; private boolean headOp; private boolean forceUpdateContainerCacheFromSCM; - private Long overwriteUpdateID = null; + private Long overwriteGeneration = null; public Builder setVolumeName(String volume) { this.volumeName = volume; @@ -333,8 +334,8 @@ public Builder setForceUpdateContainerCacheFromSCM(boolean value) { return this; } - public Builder setOverwriteUpdateID(long overwriteUpdateID) { - this.overwriteUpdateID = overwriteUpdateID; + public Builder setOverwriteGeneration(long generation) { + this.overwriteGeneration = generation; return this; } diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java index 9b833ee52e80..d284225e0380 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java @@ -100,12 +100,13 @@ public static Codec getCodec(boolean ignorePipeline) { */ private final List acls; - // OverwriteUpdateID when used in key creation indicates that a key with the same keyName - // should exist with the given updateID. - // Upon commit, the updateID of that existing key should be unchanged. - // This is a form of optimistic locking, to ensure the key is not modified between - // the time the key is read and the time the key is written. - private Long overwriteUpdateID = null; + // OverwriteGeneration, when used in key creation indicates that a + // key with the same keyName should exist with the given generation. + // For a key commit to succeed, the original key should still be present with the + // generation unchanged. + // This allows a key to be created an committed atomically if the original has not + // been modified. + private Long overwriteGeneration = null; private OmKeyInfo(Builder b) { super(b); @@ -122,7 +123,7 @@ private OmKeyInfo(Builder b) { this.fileChecksum = b.fileChecksum; this.fileName = b.fileName; this.isFile = b.isFile; - this.overwriteUpdateID = b.overwriteUpdateID; + this.overwriteGeneration = b.overwriteGeneration; } public String getVolumeName() { @@ -165,12 +166,12 @@ public String getFileName() { return fileName; } - public void setOverwriteUpdateID(Long overwriteUpdateID) { - this.overwriteUpdateID = overwriteUpdateID; + public void setOverwriteGeneration(Long generation) { + this.overwriteGeneration = generation; } - public Long getOverwriteUpdateID() { - return overwriteUpdateID; + public Long getOverwriteGeneration() { + return overwriteGeneration; } public synchronized OmKeyLocationInfoGroup getLatestVersionLocations() { @@ -443,7 +444,7 @@ public static class Builder extends WithParentObjectId.Builder { private FileChecksum fileChecksum; private boolean isFile; - private Long overwriteUpdateID = null; + private Long overwriteGeneration = null; public Builder() { } @@ -567,8 +568,8 @@ public Builder setFile(boolean isAFile) { return this; } - public Builder setOverwriteUpdateID(Long existingUpdateID) { - this.overwriteUpdateID = existingUpdateID; + public Builder setOverwriteGeneration(Long existingGeneration) { + this.overwriteGeneration = existingGeneration; return this; } @@ -676,8 +677,8 @@ private KeyInfo getProtobuf(boolean ignorePipeline, String fullKeyName, kb.setFileEncryptionInfo(OMPBHelper.convert(encInfo)); } kb.setIsFile(isFile); - if (overwriteUpdateID != null) { - kb.setOverwriteUpdateID(overwriteUpdateID); + if (overwriteGeneration != null) { + kb.setOverwriteGeneration(overwriteGeneration); } return kb.build(); } @@ -725,8 +726,8 @@ public static OmKeyInfo getFromProtobuf(KeyInfo keyInfo) throws IOException { if (keyInfo.hasIsFile()) { builder.setFile(keyInfo.getIsFile()); } - if (keyInfo.hasOverwriteUpdateID()) { - builder.setOverwriteUpdateID(keyInfo.getOverwriteUpdateID()); + if (keyInfo.hasOverwriteGeneration()) { + builder.setOverwriteGeneration(keyInfo.getOverwriteGeneration()); } // not persisted to DB. FileName will be filtered out from keyName @@ -831,8 +832,8 @@ public OmKeyInfo copyObject() { if (fileChecksum != null) { builder.setFileChecksum(fileChecksum); } - if (overwriteUpdateID != null) { - builder.setOverwriteUpdateID(overwriteUpdateID); + if (overwriteGeneration != null) { + builder.setOverwriteGeneration(overwriteGeneration); } return builder.build(); diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java index 357a880abab9..10dfef46a85e 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java @@ -734,8 +734,8 @@ public OpenKeySession openKey(OmKeyArgs args) throws IOException { keyArgs.setSortDatanodes(args.getSortDatanodes()); - if (args.getOverwriteUpdateID() != null) { - keyArgs.setOverwriteUpdateID(args.getOverwriteUpdateID()); + if (args.getOverwriteGeneration() != null) { + keyArgs.setOverwriteGeneration(args.getOverwriteGeneration()); } req.setKeyArgs(keyArgs.build()); diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java index fd3207b9e348..1c7f201c264e 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java @@ -67,7 +67,7 @@ public void protobufConversion() throws IOException { assertFalse(key.isHsync()); key.getMetadata().put(OzoneConsts.HSYNC_CLIENT_ID, "clientid"); assertTrue(key.isHsync()); - assertEquals(5678L, key.getOverwriteUpdateID()); + assertEquals(5678L, key.getOverwriteGeneration()); } @Test @@ -124,7 +124,7 @@ private OmKeyInfo createOmKeyInfo(ReplicationConfig replicationConfig) { .setReplicationConfig(replicationConfig) .addMetadata("key1", "value1") .addMetadata("key2", "value2") - .setOverwriteUpdateID(5678L) + .setOverwriteGeneration(5678L) .build(); } diff --git a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto index c8f799ebb151..4299fdadc745 100644 --- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto +++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto @@ -1025,12 +1025,13 @@ message KeyArgs { optional hadoop.hdds.ECReplicationConfig ecReplicationConfig = 19; // Force OM to update container cache location from SCL optional bool forceUpdateContainerCacheFromSCM = 20; - // Overwrite updateID when used in key creation indicates that a - // key with the same keyName should exist with the given updateID. - // Upon commit, the updateID of that existing key should be unchanged. - // This is a form of optimistic locking, to ensure the key is not modified between - // the time the key is read and the time the key is written. - optional uint64 overwriteUpdateID = 21; + // overwriteGeneration, when used in key creation indicates that a + // key with the same keyName should exist with the given generation. + // For a key commit to succeed, the original key should still be present with the + // generation unchanged. + // This allows a key to be created an committed atomically if the original has not + // been modified. + optional uint64 overwriteGeneration = 21; } message KeyLocation { @@ -1113,12 +1114,13 @@ message KeyInfo { optional hadoop.hdds.ECReplicationConfig ecReplicationConfig = 17; optional FileChecksumProto fileChecksum = 18; optional bool isFile = 19; - // Overwrite updateID when used in key creation indicates that a - // key with the same keyName should exist with the given updateID. - // Upon commit, the updateID of that existing key should be unchanged. - // This is a form of optimistic locking, to ensure the key is not modified between - // the time the key is read and the time the key is written. - optional uint64 overwriteUpdateID = 20; + // overwriteGeneration, when used in key creation indicates that a + // key with the same keyName should exist with the given generation. + // For a key commit to succeed, the original key should still be present with the + // generation unchanged. + // This allows a key to be created an committed atomically if the original has not + // been modified. + optional uint64 overwriteGeneration = 20; } message BasicKeyInfo { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java index f00a7d2c99eb..5fae8e069999 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java @@ -80,9 +80,9 @@ default Map buildKeyArgsAuditMap(KeyArgs keyArgs) { auditMap.put(OzoneConsts.REPLICATION_CONFIG, ECReplicationConfig.toString(keyArgs.getEcReplicationConfig())); } - if (keyArgs.hasOverwriteUpdateID()) { - auditMap.put(OzoneConsts.OVERWRITE_UPDATE_ID, - String.valueOf(keyArgs.getOverwriteUpdateID())); + if (keyArgs.hasOverwriteGeneration()) { + auditMap.put(OzoneConsts.OVERWRITE_GENERATION, + String.valueOf(keyArgs.getOverwriteGeneration())); } return auditMap; } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java index ca43a9e050c6..9d5f97917044 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java @@ -241,10 +241,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn "entry is not found in the OpenKey table", KEY_NOT_FOUND); } - validateOptimisticLockingOverwrite(keyToDelete, omKeyInfo, auditMap); + validateAtomicOverwrite(keyToDelete, omKeyInfo, auditMap); // Optimistic locking validation has passed. Now set the overwrite fields to null so they are // not persisted in the key table. - omKeyInfo.setOverwriteUpdateID(null); + omKeyInfo.setOverwriteGeneration(null); omKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( commitKeyArgs.getMetadataList())); @@ -504,18 +504,19 @@ public static OMRequest disallowHsync( return req; } - private void validateOptimisticLockingOverwrite(OmKeyInfo existing, OmKeyInfo toCommit, Map auditMap) + private void validateAtomicOverwrite(OmKeyInfo existing, OmKeyInfo toCommit, Map auditMap) throws OMException { - if (toCommit.getOverwriteUpdateID() != null) { - // These values are no passed in the request keyArgs, so add them into the auditMap if they are present + if (toCommit.getOverwriteGeneration() != null) { + // These values are not passed in the request keyArgs, so add them into the auditMap if they are present // in the open key entry. - auditMap.put(OzoneConsts.OVERWRITE_UPDATE_ID, String.valueOf(toCommit.getOverwriteUpdateID())); + auditMap.put(OzoneConsts.OVERWRITE_GENERATION, String.valueOf(toCommit.getOverwriteGeneration())); if (existing == null) { - throw new OMException("Overwrite with optimistic locking is not allowed for a new key", KEY_NOT_FOUND); + throw new OMException("Atomic overwrite is not allowed for a new key", KEY_NOT_FOUND); } - if (!toCommit.getOverwriteUpdateID().equals(existing.getUpdateID())) { - throw new OMException("Cannot commit as current updateID (" + existing.getUpdateID() + - ") does not match with the overwrite updateID (" + toCommit.getOverwriteUpdateID() + ")", KEY_NOT_FOUND); + if (!toCommit.getOverwriteGeneration().equals(existing.getUpdateID())) { + throw new OMException("Cannot commit as current generation (" + existing.getUpdateID() + + ") does not match with the overwrite generation (" + toCommit.getOverwriteGeneration() + ")", + KEY_NOT_FOUND); } } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java index 3cc8d8888249..26179644f038 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java @@ -231,7 +231,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn keyName); OmKeyInfo dbKeyInfo = omMetadataManager.getKeyTable(getBucketLayout()) .getIfExist(dbKeyName); - validateOptimisticLockingOverwrite(dbKeyInfo, keyArgs); + validateAtomicOverwrite(dbKeyInfo, keyArgs); OmBucketInfo bucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName); @@ -442,15 +442,15 @@ public static OMRequest blockCreateKeyWithBucketLayoutFromOldClient( return req; } - private void validateOptimisticLockingOverwrite(OmKeyInfo dbKeyInfo, KeyArgs keyArgs) + private void validateAtomicOverwrite(OmKeyInfo dbKeyInfo, KeyArgs keyArgs) throws OMException { - if (keyArgs.hasOverwriteUpdateID()) { + if (keyArgs.hasOverwriteGeneration()) { // If a key does not exist, or if it exists but the updateID do not match, then fail this request. if (dbKeyInfo == null) { throw new OMException("Key not found during expected overwrite", OMException.ResultCodes.KEY_NOT_FOUND); } - if (dbKeyInfo.getUpdateID() != keyArgs.getOverwriteUpdateID()) { - throw new OMException("UpdateID mismatch during expected overwrite", OMException.ResultCodes.KEY_NOT_FOUND); + if (dbKeyInfo.getUpdateID() != keyArgs.getOverwriteGeneration()) { + throw new OMException("Generation mismatch during expected overwrite", OMException.ResultCodes.KEY_NOT_FOUND); } } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java index 5f7a8667cc95..cca2658cb5e8 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java @@ -776,14 +776,14 @@ protected OmKeyInfo prepareFileInfo( // Construct a new metadata map from KeyArgs. // Clear the old one when the key is overwritten unless it is a key overwrite - // using optimistic commit. - if (!keyArgs.hasOverwriteUpdateID()) { + // using atomic overwrite. + if (!keyArgs.hasOverwriteGeneration()) { dbKeyInfo.getMetadata().clear(); dbKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( keyArgs.getMetadataList())); } - if (keyArgs.hasOverwriteUpdateID()) { - dbKeyInfo.setOverwriteUpdateID(keyArgs.getOverwriteUpdateID()); + if (keyArgs.hasOverwriteGeneration()) { + dbKeyInfo.setOverwriteGeneration(keyArgs.getOverwriteGeneration()); } dbKeyInfo.setFileEncryptionInfo(encInfo); return dbKeyInfo; diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java index 475f43e32a2f..dd708b12f260 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java @@ -249,7 +249,7 @@ public void testCommitWithOptimisticLocking() throws Exception { OmKeyInfo.Builder omKeyInfoBuilder = OMRequestTestUtils.createOmKeyInfo( volumeName, bucketName, keyName, replicationConfig, new OmKeyLocationInfoGroup(version, new ArrayList<>())); - omKeyInfoBuilder.setOverwriteUpdateID(1L); + omKeyInfoBuilder.setOverwriteGeneration(1L); OmKeyInfo omKeyInfo = omKeyInfoBuilder.build(); omKeyInfo.appendNewBlocks(allocatedLocationList, false); List acls = Collections.singletonList(OzoneAcl.parseAcl("user:foo:rw")); @@ -261,18 +261,18 @@ public void testCommitWithOptimisticLocking() throws Exception { OmKeyInfo openKeyInfo = openKeyTable.get(openKey); assertNotNull(openKeyInfo); assertEquals(acls, openKeyInfo.getAcls()); - // At this stage, we have an openKey, with overwrite update ID of 1. + // At this stage, we have an openKey, with overwrite generation of 1. // However there is no closed key entry, so the commit should fail. OMClientResponse omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); assertEquals(KEY_NOT_FOUND, omClientResponse.getOMResponse().getStatus()); - // Now add the key to the key table, and try again, but with different update_ID - omKeyInfoBuilder.setOverwriteUpdateID(null); + // Now add the key to the key table, and try again, but with different generation + omKeyInfoBuilder.setOverwriteGeneration(null); omKeyInfoBuilder.setUpdateID(0L); OmKeyInfo invalidKeyInfo = omKeyInfoBuilder.build(); closedKeyTable.put(getOzonePathKey(), invalidKeyInfo); - // This should fail as the IDs are zero and the open key has overwrite IDs of 1. + // This should fail as the updateID ia zero and the open key has overwrite generation of 1. omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); assertEquals(KEY_NOT_FOUND, omClientResponse.getOMResponse().getStatus()); @@ -282,12 +282,12 @@ public void testCommitWithOptimisticLocking() throws Exception { closedKeyTable.delete(getOzonePathKey()); closedKeyTable.put(getOzonePathKey(), closedKeyInfo); - // Now the key should commit as the IDs match. + // Now the key should commit as the updateID and overwriteGeneration match. omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); assertEquals(OK, omClientResponse.getOMResponse().getStatus()); OmKeyInfo committedKey = closedKeyTable.get(getOzonePathKey()); - assertNull(committedKey.getOverwriteUpdateID()); + assertNull(committedKey.getOverwriteGeneration()); // Update ID should be changed assertNotEquals(closedKeyInfo.getUpdateID(), committedKey.getUpdateID()); assertEquals(acls, committedKey.getAcls()); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java index cc6bc1ecb2b1..38e1266bb4ba 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java @@ -706,7 +706,7 @@ protected OMRequest createKeyRequest(boolean isMultipartKey, int partNumber, private OMRequest createKeyRequest( boolean isMultipartKey, int partNumber, long keyLength, - ReplicationConfig repConfig, Long overwriteUpdateID) { + ReplicationConfig repConfig, Long overwriteGeneration) { KeyArgs.Builder keyArgs = KeyArgs.newBuilder() .setVolumeName(volumeName).setBucketName(bucketName) @@ -725,8 +725,8 @@ private OMRequest createKeyRequest( if (isMultipartKey) { keyArgs.setMultipartNumber(partNumber); } - if (overwriteUpdateID != null) { - keyArgs.setOverwriteUpdateID(overwriteUpdateID); + if (overwriteGeneration != null) { + keyArgs.setOverwriteGeneration(overwriteGeneration); } OzoneManagerProtocolProtos.CreateKeyRequest createKeyRequest = @@ -949,7 +949,7 @@ public void testOptimisticOverwrite( List existingAcls = existingKeyInfo.getAcls(); assertEquals(acls, existingAcls); - // Create a request with an update ID which doesn't match the current key + // Create a request with an generation which doesn't match the current key omRequest = createKeyRequest(false, 0, 100, RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getUpdateID() + 1); omRequest = doPreExecute(omRequest); @@ -958,7 +958,7 @@ public void testOptimisticOverwrite( // Still fails, as the matching key is not present. assertEquals(KEY_NOT_FOUND, response.getOMResponse().getStatus()); - // Now create the key with the correct overwrite IDs + // Now create the key with the correct overwrite generation omRequest = createKeyRequest(false, 0, 100, RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getUpdateID()); omRequest = doPreExecute(omRequest); @@ -966,12 +966,12 @@ public void testOptimisticOverwrite( response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); assertEquals(OK, response.getOMResponse().getStatus()); - // Ensure the updateID is persisted in the open key table + // Ensure the overwriteGeneration is persisted in the open key table String openKey = omMetadataManager.getOpenKey(volumeName, bucketName, keyName, omRequest.getCreateKeyRequest().getClientID()); OmKeyInfo openKeyInfo = omMetadataManager.getOpenKeyTable(omKeyCreateRequest.getBucketLayout()).get(openKey); - assertEquals(existingKeyInfo.getUpdateID(), openKeyInfo.getOverwriteUpdateID()); + assertEquals(existingKeyInfo.getUpdateID(), openKeyInfo.getOverwriteGeneration()); // Creation time should remain the same on overwrite. assertEquals(existingKeyInfo.getCreationTime(), openKeyInfo.getCreationTime()); // Update ID should change From bcd1a284178bc54e6ad844027de7b948b9124642 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Tue, 30 Apr 2024 15:11:27 +0100 Subject: [PATCH 19/26] Remove updateID references from client and replace with generation --- .../apache/hadoop/ozone/client/OzoneKey.java | 22 +++++++++---------- .../hadoop/ozone/client/OzoneKeyDetails.java | 4 ++-- .../hadoop/ozone/client/rpc/RpcClient.java | 11 +++++----- .../hadoop/ozone/om/helpers/WithObjectID.java | 7 ++++++ .../hadoop/ozone/client/OzoneBucketStub.java | 4 ++-- 5 files changed, 26 insertions(+), 22 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java index 5e61114a0674..05f5db94a9e9 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java @@ -72,17 +72,16 @@ public class OzoneKey { */ /** - * The update ID of an existing key. This will be null if this OzoneKey - * object has not been created from an existing key read from OM, as OM allocates - * the update ID on commit of the key. + * The generation of an existing key. This can be used with atomic commits, to + * ensure the key has not changed since the key details were read. */ - private final Long updateID; + private final Long generation; @SuppressWarnings("parameternumber") public OzoneKey(String volumeName, String bucketName, String keyName, long size, long creationTime, long modificationTime, ReplicationConfig replicationConfig, - boolean isFile, Long updateID) { + boolean isFile, Long generation) { this.volumeName = volumeName; this.bucketName = bucketName; this.name = keyName; @@ -91,7 +90,7 @@ public OzoneKey(String volumeName, String bucketName, this.modificationTime = Instant.ofEpochMilli(modificationTime); this.replicationConfig = replicationConfig; this.isFile = isFile; - this.updateID = updateID; + this.generation = generation; } @SuppressWarnings("parameternumber") @@ -117,9 +116,9 @@ public OzoneKey(String volumeName, String bucketName, public OzoneKey(String volumeName, String bucketName, String keyName, long size, long creationTime, long modificationTime, ReplicationConfig replicationConfig, - Map metadata, boolean isFile, Long updateID) { + Map metadata, boolean isFile, Long generation) { this(volumeName, bucketName, keyName, size, creationTime, - modificationTime, replicationConfig, isFile, updateID); + modificationTime, replicationConfig, isFile, generation); this.metadata.putAll(metadata); } /** @@ -206,9 +205,8 @@ public ReplicationConfig getReplicationConfig() { return replicationConfig; } - @JsonIgnore - public Long getUpdateID() { - return updateID; + public Long getGeneration() { + return generation; } /** @@ -223,7 +221,7 @@ public static OzoneKey fromKeyInfo(OmKeyInfo keyInfo) { return new OzoneKey(keyInfo.getVolumeName(), keyInfo.getBucketName(), keyInfo.getKeyName(), keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), keyInfo.getReplicationConfig(), - keyInfo.getMetadata(), keyInfo.isFile(), keyInfo.getUpdateID()); + keyInfo.getMetadata(), keyInfo.isFile(), keyInfo.getGeneration()); } } diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java index c86da1d778c1..cac43e907899 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java @@ -53,9 +53,9 @@ public OzoneKeyDetails(String volumeName, String bucketName, String keyName, Map metadata, FileEncryptionInfo feInfo, CheckedSupplier contentSupplier, - boolean isFile, Long updateID) { + boolean isFile, Long generation) { super(volumeName, bucketName, keyName, size, creationTime, - modificationTime, replicationConfig, metadata, isFile, updateID); + modificationTime, replicationConfig, metadata, isFile, generation); this.ozoneKeyLocations = ozoneKeyLocations; this.feInfo = feInfo; this.contentSupplier = contentSupplier; diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 1721e132ba4f..522497118c06 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1404,8 +1404,8 @@ public OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, Replicatio if (keyToOverwrite == null) { throw new IllegalArgumentException("KeyToOverwrite cannot be null"); } - if (keyToOverwrite.getUpdateID() == null) { - throw new IllegalArgumentException("KeyToOverwrite UpdateID cannot be null"); + if (keyToOverwrite.getGeneration() == null) { + throw new IllegalArgumentException("KeyToOverwrite generation cannot be null"); } createKeyPreChecks(keyToOverwrite.getVolumeName(), keyToOverwrite.getBucketName(), keyToOverwrite.getName(), replicationConfig); @@ -1418,8 +1418,7 @@ public OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, Replicatio .setReplicationConfig(replicationConfig) .addAllMetadataGdpr(keyToOverwrite.getMetadata()) .setLatestVersionLocation(getLatestVersionLocation) - // STODO - keyToOverwrite should have a getGeneration method rather than updateID - .setOverwriteGeneration(keyToOverwrite.getUpdateID()); + .setOverwriteGeneration(keyToOverwrite.getGeneration()); OpenKeySession openKey = ozoneManagerClient.openKey(builder.build()); // For bucket with layout OBJECT_STORE, when create an empty file (size=0), @@ -1693,7 +1692,7 @@ public List listKeys(String volumeName, String bucketName, key.getModificationTime(), key.getReplicationConfig(), key.isFile(), - key.getUpdateID())) + key.getGeneration())) .collect(Collectors.toList()); } } @@ -1744,7 +1743,7 @@ private OzoneKeyDetails getOzoneKeyDetails(OmKeyInfo keyInfo) { keyInfo.getModificationTime(), ozoneKeyLocations, keyInfo.getReplicationConfig(), keyInfo.getMetadata(), keyInfo.getFileEncryptionInfo(), - () -> getInputStreamWithRetryFunction(keyInfo), keyInfo.isFile(), keyInfo.getUpdateID()); + () -> getInputStreamWithRetryFunction(keyInfo), keyInfo.isFile(), keyInfo.getGeneration()); } @Override diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithObjectID.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithObjectID.java index af9508196260..8647c7ffa6ca 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithObjectID.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithObjectID.java @@ -53,6 +53,13 @@ public final long getUpdateID() { return updateID; } + /** + * Returns the generation of the object. Note this is currently the same as updateID. + * @return long + */ + public final long getGeneration() { + return getUpdateID(); + } /** * Set the Object ID. * There is a reason why we cannot use the final here. The object diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java index 14b894479962..8510b929a89b 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java @@ -307,7 +307,7 @@ public OzoneKey headObject(String key) throws IOException { ozoneKeyDetails.getCreationTime().toEpochMilli(), ozoneKeyDetails.getModificationTime().toEpochMilli(), ozoneKeyDetails.getReplicationConfig(), - ozoneKeyDetails.isFile(), ozoneKeyDetails.getUpdateID()); + ozoneKeyDetails.isFile(), ozoneKeyDetails.getGeneration()); } else { throw new OMException(ResultCodes.KEY_NOT_FOUND); } @@ -362,7 +362,7 @@ public Iterator listKeys(String keyPrefix, key.getDataSize(), key.getCreationTime().getEpochSecond() * 1000, key.getModificationTime().getEpochSecond() * 1000, - key.getReplicationConfig(), key.isFile(), key.getUpdateID()); + key.getReplicationConfig(), key.isFile(), key.getGeneration()); }).collect(Collectors.toList()); if (prevKey != null) { From 4e6a97c87f14864cb50904732041afb90973526b Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Tue, 30 Apr 2024 17:51:00 +0100 Subject: [PATCH 20/26] Change client API to rewriteKey --- .../hadoop/ozone/client/OzoneBucket.java | 30 +++++++++---------- .../ozone/client/protocol/ClientProtocol.java | 26 ++++++++++------ .../hadoop/ozone/client/rpc/RpcClient.java | 30 ++++++++----------- .../rpc/TestOzoneRpcClientAbstract.java | 12 ++++---- .../ozone/client/ClientProtocolStub.java | 9 +++--- .../hadoop/ozone/client/OzoneBucketStub.java | 18 +++++------ 6 files changed, 65 insertions(+), 60 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index 5ae6b93b8d6e..c72abde6f4b8 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -470,27 +470,25 @@ public OzoneOutputStream createKey(String key, long size, } /** - * Overwrite an existing key using optimistic locking. The existingKey must exist in Ozone to allow - * the new key to be created with the same name. Additionally, the existing Key must not have been - * modified since the time it's details were read. This is controlled by the updateID - * field in the existing Key. If the key is replaced or updated the updateID will change. If the - * updateID has changed since the existing Key was read, either the initial key create will fail, + * This API allows to atomically update an existing key. The key read before invoking this API + * should remain unchanged for this key to be written. This is controlled by the generation + * field in the existing Key param. If the key is replaced or updated the generation will change. If the + * generation has changed since the existing Key was read, either the initial key create will fail, * or the key will fail to commit after the data has been written as the checks are carried out - * both a key open and commit time. + * both at key open and commit time. * - * For now this feature only works on Object Store Buckets. FSO support will be added a later. - * - * @param existingKey Name of the key to be created. - * @param replicationConfig Replication configuration. + * @param keyName Existing key to overwrite. This must exist in the bucket. + * @param size The size of the new key + * @param existingKeyGeneration The generation of the existing key which is checked for changes at key create + * and commit time. + * @param replicationConfig The replication configuration for the key to be rewritten. + * @param metadata custom key value metadata * @return OzoneOutputStream to which the data has to be written. * @throws IOException */ - public OzoneOutputStream overwriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig) - throws IOException { - if (this.bucketLayout != BucketLayout.OBJECT_STORE) { - throw new IllegalArgumentException("Optimistic locking is only supported on Object Store Buckets"); - } - return proxy.overwriteKey(existingKey, replicationConfig); + public OzoneOutputStream rewriteKey(String keyName, long size, long existingKeyGeneration, + ReplicationConfig replicationConfig, Map metadata) throws IOException { + return proxy.rewriteKey(volumeName, name, keyName, size, existingKeyGeneration, replicationConfig, metadata); } /** diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java index 6e8f3237f815..2ec6d77384b1 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java @@ -354,19 +354,27 @@ OzoneOutputStream createKey(String volumeName, String bucketName, throws IOException; /** - * Overwrite an existing key using optimistic locking. The OzoneKeyDetails passed must contain - * the updateID of the key to be overwritten. The existing key must also exist on - * OM and have the same UpdateID as the one passed in or the key create / commit will fail. + * This API allows to atomically update an existing key. The key read before invoking this API + * should remain unchanged for this key to be written. This is controlled by the generation + * field in the existing Key param. If the key is replaced or updated the generation will change. If the + * generation has changed since the existing Key was read, either the initial key create will fail, + * or the key will fail to commit after the data has been written as the checks are carried out + * both at key open and commit time. * - * Currently does not work for FSO buckets. Support will be added later. - * - * @param keyToOverwrite Existing key to overwrite - * @param replicationConfig The replication configuration for the new key + * @param volumeName Name of the Volume + * @param bucketName Name of the Bucket + * @param keyName Existing key to overwrite. This must exist in the bucket. + * @param size The size of the new key + * @param existingKeyGeneration The generation of the existing key which is checked for changes at key create + * and commit time. + * @param replicationConfig The replication configuration for the key to be rewritten. + * @param metadata custom key value metadata * @return {@link OzoneOutputStream} * @throws IOException */ - OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig) - throws IOException; + OzoneOutputStream rewriteKey(String volumeName, String bucketName, String keyName, + long size, long existingKeyGeneration, ReplicationConfig replicationConfig, + Map metadata) throws IOException; /** * Writes a key in an existing bucket. diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 522497118c06..61684f37a0dd 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1397,35 +1397,31 @@ public OzoneOutputStream createKey( return createOutputStream(openKey); } - // STODO - rename this API and params. @Override - public OzoneOutputStream overwriteKey(OzoneKeyDetails keyToOverwrite, ReplicationConfig replicationConfig) - throws IOException { - if (keyToOverwrite == null) { - throw new IllegalArgumentException("KeyToOverwrite cannot be null"); - } - if (keyToOverwrite.getGeneration() == null) { - throw new IllegalArgumentException("KeyToOverwrite generation cannot be null"); + public OzoneOutputStream rewriteKey(String volumeName, String bucketName, String keyName, + long size, long existingKeyGeneration, ReplicationConfig replicationConfig, + Map metadata) throws IOException { + if (keyName == null) { + throw new IllegalArgumentException("Key cannot be null"); } - createKeyPreChecks(keyToOverwrite.getVolumeName(), keyToOverwrite.getBucketName(), - keyToOverwrite.getName(), replicationConfig); + createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig); OmKeyArgs.Builder builder = new OmKeyArgs.Builder() - .setVolumeName(keyToOverwrite.getVolumeName()) - .setBucketName(keyToOverwrite.getBucketName()) - .setKeyName(keyToOverwrite.getName()) - .setDataSize(keyToOverwrite.getDataSize()) + .setVolumeName(volumeName) + .setBucketName(bucketName) + .setKeyName(keyName) + .setDataSize(size) .setReplicationConfig(replicationConfig) - .addAllMetadataGdpr(keyToOverwrite.getMetadata()) + .addAllMetadataGdpr(metadata) .setLatestVersionLocation(getLatestVersionLocation) - .setOverwriteGeneration(keyToOverwrite.getGeneration()); + .setOverwriteGeneration(existingKeyGeneration); OpenKeySession openKey = ozoneManagerClient.openKey(builder.build()); // For bucket with layout OBJECT_STORE, when create an empty file (size=0), // OM will set DataSize to OzoneConfigKeys#OZONE_SCM_BLOCK_SIZE, // which will cause S3G's atomic write length check to fail, // so reset size to 0 here. - if (isS3GRequest.get() && keyToOverwrite.getDataSize() == 0) { + if (isS3GRequest.get() && size == 0) { openKey.getKeyInfo().setDataSize(0); } return createOutputStream(openKey); diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java index 49490a68856c..7b6b9aa156d3 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java @@ -1058,8 +1058,9 @@ public void testOverwriteKey() throws IOException { } OzoneKeyDetails keyDetails = bucket.getKey(keyName); - try (OzoneOutputStream out = bucket.overwriteKey( - keyDetails, RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE))) { + try (OzoneOutputStream out = bucket.rewriteKey(keyDetails.getName(), keyDetails.getDataSize(), + keyDetails.getGeneration(), RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE), + keyDetails.getMetadata())) { out.write(overwriteValue.getBytes(UTF_8)); } @@ -1072,10 +1073,11 @@ public void testOverwriteKey() throws IOException { // Delete the key bucket.deleteKey(keyName); - // Now try the over write again, and it should fail as the originally read key is no longer there. + // Now try the overwrite again, and it should fail as the originally read key is no longer there. assertThrows(IOException.class, () -> { - try (OzoneOutputStream out = bucket.overwriteKey( - keyDetails, RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE))) { + try (OzoneOutputStream out = bucket.rewriteKey(keyDetails.getName(), keyDetails.getDataSize(), + keyDetails.getGeneration(), RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE), + keyDetails.getMetadata())) { out.write(overwriteValue.getBytes(UTF_8)); } }); diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java index 19869faa402e..5bc4d95b5c2a 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java @@ -227,10 +227,11 @@ public OzoneOutputStream createKey(String volumeName, String bucketName, } @Override - public OzoneOutputStream overwriteKey(OzoneKeyDetails existingKey, ReplicationConfig replicationConfig) - throws IOException { - return getBucket(existingKey.getVolumeName(), existingKey.getBucketName()) - .overwriteKey(existingKey, replicationConfig); + public OzoneOutputStream rewriteKey(String volumeName, String bucketName, String keyName, + long size, long existingKeyGeneration, ReplicationConfig replicationConfig, + Map metadata) throws IOException { + return getBucket(volumeName, bucketName) + .rewriteKey(keyName, size, existingKeyGeneration, replicationConfig, metadata); } @Override diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java index 8510b929a89b..c19fdd7b9bc0 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java @@ -155,8 +155,8 @@ public void close() throws IOException { } @Override - public OzoneOutputStream overwriteKey(OzoneKeyDetails existingKey, ReplicationConfig rConfig) - throws IOException { + public OzoneOutputStream rewriteKey(String keyName, long size, long existingKeyGeneration, + ReplicationConfig rConfig, Map metadata) throws IOException { final ReplicationConfig repConfig; if (rConfig == null) { repConfig = getReplicationConfig(); @@ -165,19 +165,19 @@ public OzoneOutputStream overwriteKey(OzoneKeyDetails existingKey, ReplicationCo } ReplicationConfig finalReplicationCon = repConfig; ByteArrayOutputStream byteArrayOutputStream = - new KeyMetadataAwareOutputStream(existingKey.getMetadata()) { + new KeyMetadataAwareOutputStream(metadata) { @Override public void close() throws IOException { - keyContents.put(existingKey.getName(), toByteArray()); - keyDetails.put(existingKey.getName(), new OzoneKeyDetails( + keyContents.put(keyName, toByteArray()); + keyDetails.put(keyName, new OzoneKeyDetails( getVolumeName(), getName(), - existingKey.getName(), - existingKey.getDataSize(), + keyName, + size, System.currentTimeMillis(), System.currentTimeMillis(), - new ArrayList<>(), finalReplicationCon, existingKey.getMetadata(), null, - () -> readKey(existingKey.getName()), true + new ArrayList<>(), finalReplicationCon, metadata, null, + () -> readKey(keyName), true )); super.close(); } From 247620afc4214b27b13adfee5085bd6fd330f496 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Tue, 30 Apr 2024 18:15:21 +0100 Subject: [PATCH 21/26] Fixed logic to not copy meta data from existing key, but take it from the client as with create key --- .../ozone/om/request/key/OMKeyRequest.java | 11 +++---- .../request/key/TestOMKeyCommitRequest.java | 6 ++-- .../request/key/TestOMKeyCreateRequest.java | 29 ++++++++++++++----- 3 files changed, 29 insertions(+), 17 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java index cca2658cb5e8..fc20c7da871c 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java @@ -775,13 +775,10 @@ protected OmKeyInfo prepareFileInfo( dbKeyInfo.setReplicationConfig(replicationConfig); // Construct a new metadata map from KeyArgs. - // Clear the old one when the key is overwritten unless it is a key overwrite - // using atomic overwrite. - if (!keyArgs.hasOverwriteGeneration()) { - dbKeyInfo.getMetadata().clear(); - dbKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( - keyArgs.getMetadataList())); - } + dbKeyInfo.getMetadata().clear(); + dbKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( + keyArgs.getMetadataList())); + if (keyArgs.hasOverwriteGeneration()) { dbKeyInfo.setOverwriteGeneration(keyArgs.getOverwriteGeneration()); } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java index dd708b12f260..9d48bd240d73 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java @@ -225,7 +225,7 @@ public void testValidateAndUpdateCache() throws Exception { } @Test - public void testCommitWithOptimisticLocking() throws Exception { + public void testAtomicRewrite() throws Exception { if (getBucketLayout() == BucketLayout.FILE_SYSTEM_OPTIMIZED) { // TODO - does not work with in FSO for now return; @@ -288,8 +288,8 @@ public void testCommitWithOptimisticLocking() throws Exception { OmKeyInfo committedKey = closedKeyTable.get(getOzonePathKey()); assertNull(committedKey.getOverwriteGeneration()); - // Update ID should be changed - assertNotEquals(closedKeyInfo.getUpdateID(), committedKey.getUpdateID()); + // Generation should be changed + assertNotEquals(closedKeyInfo.getGeneration(), committedKey.getGeneration()); assertEquals(acls, committedKey.getAcls()); } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java index 38e1266bb4ba..0302bfecb759 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java @@ -707,6 +707,13 @@ protected OMRequest createKeyRequest(boolean isMultipartKey, int partNumber, private OMRequest createKeyRequest( boolean isMultipartKey, int partNumber, long keyLength, ReplicationConfig repConfig, Long overwriteGeneration) { + return createKeyRequest(isMultipartKey, partNumber, keyLength, repConfig, + overwriteGeneration, null); + } + + private OMRequest createKeyRequest( + boolean isMultipartKey, int partNumber, long keyLength, + ReplicationConfig repConfig, Long overwriteGeneration, Map metaData) { KeyArgs.Builder keyArgs = KeyArgs.newBuilder() .setVolumeName(volumeName).setBucketName(bucketName) @@ -728,6 +735,12 @@ private OMRequest createKeyRequest( if (overwriteGeneration != null) { keyArgs.setOverwriteGeneration(overwriteGeneration); } + if (metaData != null) { + metaData.forEach((key, value) -> keyArgs.addMetadata(KeyValue.newBuilder() + .setKey(key) + .setValue(value) + .build())); + } OzoneManagerProtocolProtos.CreateKeyRequest createKeyRequest = CreateKeyRequest.newBuilder().setKeyArgs(keyArgs).build(); @@ -913,7 +926,7 @@ public void testKeyCreateInheritParentDefaultAcls( @ParameterizedTest @MethodSource("data") - public void testOptimisticOverwrite( + public void testAtomicRewrite( boolean setKeyPathLock, boolean setFileSystemPaths) throws Exception { when(ozoneManager.getOzoneLockProvider()).thenReturn( new OzoneLockProvider(setKeyPathLock, setFileSystemPaths)); @@ -938,6 +951,8 @@ public void testOptimisticOverwrite( // Now pre-create the key in the system so we can overwrite it. Map metadata = Collections.singletonMap("metakey", "metavalue"); + Map reWriteMetadata = Collections.singletonMap("metakey", "rewriteMetavalue"); + List acls = Collections.singletonList(OzoneAcl.parseAcl("user:foo:rw")); OmKeyInfo createdKeyInfo = createAndCheck(keyName, metadata, acls); // Commit openKey entry. @@ -951,7 +966,7 @@ public void testOptimisticOverwrite( // Create a request with an generation which doesn't match the current key omRequest = createKeyRequest(false, 0, 100, - RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getUpdateID() + 1); + RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getGeneration() + 1, reWriteMetadata); omRequest = doPreExecute(omRequest); omKeyCreateRequest = getOMKeyCreateRequest(omRequest); response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); @@ -960,7 +975,7 @@ public void testOptimisticOverwrite( // Now create the key with the correct overwrite generation omRequest = createKeyRequest(false, 0, 100, - RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getUpdateID()); + RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getGeneration(), reWriteMetadata); omRequest = doPreExecute(omRequest); omKeyCreateRequest = getOMKeyCreateRequest(omRequest); response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); @@ -971,14 +986,14 @@ public void testOptimisticOverwrite( keyName, omRequest.getCreateKeyRequest().getClientID()); OmKeyInfo openKeyInfo = omMetadataManager.getOpenKeyTable(omKeyCreateRequest.getBucketLayout()).get(openKey); - assertEquals(existingKeyInfo.getUpdateID(), openKeyInfo.getOverwriteGeneration()); + assertEquals(existingKeyInfo.getGeneration(), openKeyInfo.getOverwriteGeneration()); // Creation time should remain the same on overwrite. assertEquals(existingKeyInfo.getCreationTime(), openKeyInfo.getCreationTime()); // Update ID should change - assertNotEquals(existingKeyInfo.getUpdateID(), openKeyInfo.getUpdateID()); - // The metadata should be copied over from the existing key. + assertNotEquals(existingKeyInfo.getGeneration(), openKeyInfo.getGeneration()); assertEquals(metadata, existingKeyInfo.getMetadata()); - assertEquals(metadata, openKeyInfo.getMetadata()); + // The metadata should not be copied from the existing key. It should be passed in the request. + assertEquals(reWriteMetadata, openKeyInfo.getMetadata()); // Ensure the ACLS are copied over from the existing key. assertEquals(existingAcls, openKeyInfo.getAcls()); } From b91a0519b0c004d0e47c7e5b499cc8e8e7e3a4a2 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Tue, 7 May 2024 17:30:24 +0100 Subject: [PATCH 22/26] Review comments --- .../java/org/apache/hadoop/ozone/client/OzoneKey.java | 9 ++++----- .../org/apache/hadoop/ozone/client/rpc/RpcClient.java | 6 +----- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java index 6a4acfd555a3..490986959c37 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java @@ -70,11 +70,6 @@ public class OzoneKey { */ private final boolean isFile; - /** - * Constructs OzoneKey from OmKeyInfo. - * - */ - /** * The generation of an existing key. This can be used with atomic commits, to * ensure the key has not changed since the key details were read. @@ -231,6 +226,10 @@ public boolean isFile() { return isFile; } + /** + * Constructs OzoneKey from OmKeyInfo. + * + */ public static OzoneKey fromKeyInfo(OmKeyInfo keyInfo) { return new OzoneKey(keyInfo.getVolumeName(), keyInfo.getBucketName(), keyInfo.getKeyName(), keyInfo.getDataSize(), keyInfo.getCreationTime(), diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 742d174fd1f8..2bd2b28088d0 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1416,9 +1416,6 @@ public OzoneOutputStream createKey( public OzoneOutputStream rewriteKey(String volumeName, String bucketName, String keyName, long size, long existingKeyGeneration, ReplicationConfig replicationConfig, Map metadata) throws IOException { - if (keyName == null) { - throw new IllegalArgumentException("Key cannot be null"); - } createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig); String ownerName = getRealUserInfo().getShortUserName(); @@ -1708,8 +1705,7 @@ public List listKeys(String volumeName, String bucketName, key.getModificationTime(), key.getReplicationConfig(), key.isFile(), - key.getOwnerName(), - key.getGeneration())) + key.getOwnerName())) .collect(Collectors.toList()); } } From 9dc6d315afffa0670954f3e5366c43303c0681cc Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Thu, 9 May 2024 17:24:25 +0100 Subject: [PATCH 23/26] Change BucketLayout.DEFAULT to getBucketLayout() --- .../hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java index 9d48bd240d73..0b1bebb9ce28 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java @@ -231,8 +231,8 @@ public void testAtomicRewrite() throws Exception { return; } - Table openKeyTable = omMetadataManager.getOpenKeyTable(BucketLayout.DEFAULT); - Table closedKeyTable = omMetadataManager.getKeyTable(BucketLayout.DEFAULT); + Table openKeyTable = omMetadataManager.getOpenKeyTable(getBucketLayout()); + Table closedKeyTable = omMetadataManager.getKeyTable(getBucketLayout()); OMRequest modifiedOmRequest = doPreExecute(createCommitKeyRequest()); OMKeyCommitRequest omKeyCommitRequest = getOmKeyCommitRequest(modifiedOmRequest); From 80e180234319a9a80258763c669431e6ac3aad47 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Thu, 9 May 2024 17:32:17 +0100 Subject: [PATCH 24/26] Move getGeneration to OMKeyInfo --- .../org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java | 8 ++++++++ .../org/apache/hadoop/ozone/om/helpers/WithObjectID.java | 7 ------- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java index 9e9743b7aaab..05eb82393b03 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java @@ -181,6 +181,14 @@ public String getOwnerName() { return ownerName; } + /** + * Returns the generation of the object. Note this is currently the same as updateID for a key. + * @return long + */ + public long getGeneration() { + return getUpdateID(); + } + public synchronized OmKeyLocationInfoGroup getLatestVersionLocations() { return keyLocationVersions.size() == 0 ? null : keyLocationVersions.get(keyLocationVersions.size() - 1); diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithObjectID.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithObjectID.java index 8647c7ffa6ca..af9508196260 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithObjectID.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/WithObjectID.java @@ -53,13 +53,6 @@ public final long getUpdateID() { return updateID; } - /** - * Returns the generation of the object. Note this is currently the same as updateID. - * @return long - */ - public final long getGeneration() { - return getUpdateID(); - } /** * Set the Object ID. * There is a reason why we cannot use the final here. The object From bb7b74f31361473aebcf33c8611dff5d1fceca2d Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Thu, 9 May 2024 18:17:51 +0100 Subject: [PATCH 25/26] Rename any instances of overwrite to rewrite --- .../org/apache/hadoop/ozone/OzoneConsts.java | 2 +- .../hadoop/ozone/client/OzoneBucket.java | 2 +- .../ozone/client/protocol/ClientProtocol.java | 2 +- .../hadoop/ozone/client/rpc/RpcClient.java | 2 +- .../hadoop/ozone/om/helpers/OmKeyArgs.java | 24 +++++++------- .../hadoop/ozone/om/helpers/OmKeyInfo.java | 32 +++++++++---------- ...ManagerProtocolClientSideTranslatorPB.java | 4 +-- .../ozone/om/helpers/TestOmKeyInfo.java | 4 +-- .../rpc/TestOzoneRpcClientAbstract.java | 14 ++++---- .../src/main/proto/OmClientProtocol.proto | 8 ++--- .../ozone/om/request/RequestAuditor.java | 6 ++-- .../om/request/key/OMKeyCommitRequest.java | 18 +++++------ .../om/request/key/OMKeyCreateRequest.java | 12 +++---- .../ozone/om/request/key/OMKeyRequest.java | 4 +-- .../request/key/TestOMKeyCommitRequest.java | 12 +++---- .../request/key/TestOMKeyCreateRequest.java | 26 +++++++-------- 16 files changed, 86 insertions(+), 86 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java index 6e15804450ad..e29aeac1e12a 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java @@ -343,7 +343,7 @@ private OzoneConsts() { public static final String BUCKET_LAYOUT = "bucketLayout"; public static final String TENANT = "tenant"; public static final String USER_PREFIX = "userPrefix"; - public static final String OVERWRITE_GENERATION = "overwriteGeneration"; + public static final String REWRITE_GENERATION = "rewriteGeneration"; // For multi-tenancy public static final String TENANT_ID_USERNAME_DELIMITER = "$"; diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index be28dd1c8678..c36976fe5e63 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -478,7 +478,7 @@ public OzoneOutputStream createKey(String key, long size, * or the key will fail to commit after the data has been written as the checks are carried out * both at key open and commit time. * - * @param keyName Existing key to overwrite. This must exist in the bucket. + * @param keyName Existing key to rewrite. This must exist in the bucket. * @param size The size of the new key * @param existingKeyGeneration The generation of the existing key which is checked for changes at key create * and commit time. diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java index 314a567dbbe5..0725b4f25349 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java @@ -363,7 +363,7 @@ OzoneOutputStream createKey(String volumeName, String bucketName, * * @param volumeName Name of the Volume * @param bucketName Name of the Bucket - * @param keyName Existing key to overwrite. This must exist in the bucket. + * @param keyName Existing key to rewrite. This must exist in the bucket. * @param size The size of the new key * @param existingKeyGeneration The generation of the existing key which is checked for changes at key create * and commit time. diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java index 2bd2b28088d0..000a19415010 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java @@ -1428,7 +1428,7 @@ public OzoneOutputStream rewriteKey(String volumeName, String bucketName, String .addAllMetadataGdpr(metadata) .setLatestVersionLocation(getLatestVersionLocation) .setOwnerName(ownerName) - .setOverwriteGeneration(existingKeyGeneration); + .setRewriteGeneration(existingKeyGeneration); OpenKeySession openKey = ozoneManagerClient.openKey(builder.build()); // For bucket with layout OBJECT_STORE, when create an empty file (size=0), diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java index 71241b9a311a..35417c0b3269 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyArgs.java @@ -53,13 +53,13 @@ public final class OmKeyArgs implements Auditable { private final boolean recursive; private final boolean headOp; private final boolean forceUpdateContainerCacheFromSCM; - // OverwriteGeneration, when used in key creation indicates that a + // RewriteGeneration, when used in key creation indicates that a // key with the same keyName should exist with the given generation. // For a key commit to succeed, the original key should still be present with the // generation unchanged. // This allows a key to be created an committed atomically if the original has not // been modified. - private Long overwriteGeneration = null; + private Long rewriteGeneration = null; private OmKeyArgs(Builder b) { this.volumeName = b.volumeName; @@ -79,7 +79,7 @@ private OmKeyArgs(Builder b) { this.headOp = b.headOp; this.forceUpdateContainerCacheFromSCM = b.forceUpdateContainerCacheFromSCM; this.ownerName = b.ownerName; - this.overwriteGeneration = b.overwriteGeneration; + this.rewriteGeneration = b.rewriteGeneration; } public boolean getIsMultipartKey() { @@ -158,8 +158,8 @@ public boolean isForceUpdateContainerCacheFromSCM() { return forceUpdateContainerCacheFromSCM; } - public Long getOverwriteGeneration() { - return overwriteGeneration; + public Long getRewriteGeneration() { + return rewriteGeneration; } @Override @@ -203,8 +203,8 @@ public OmKeyArgs.Builder toBuilder() { .setAcls(acls) .setForceUpdateContainerCacheFromSCM(forceUpdateContainerCacheFromSCM); - if (overwriteGeneration != null) { - builder.setOverwriteGeneration(overwriteGeneration); + if (rewriteGeneration != null) { + builder.setRewriteGeneration(rewriteGeneration); } return builder; } @@ -221,8 +221,8 @@ public KeyArgs toProtobuf() { .setHeadOp(isHeadOp()) .setForceUpdateContainerCacheFromSCM( isForceUpdateContainerCacheFromSCM()); - if (overwriteGeneration != null) { - builder.setOverwriteGeneration(overwriteGeneration); + if (rewriteGeneration != null) { + builder.setRewriteGeneration(rewriteGeneration); } return builder.build(); } @@ -248,7 +248,7 @@ public static class Builder { private boolean recursive; private boolean headOp; private boolean forceUpdateContainerCacheFromSCM; - private Long overwriteGeneration = null; + private Long rewriteGeneration = null; public Builder setVolumeName(String volume) { this.volumeName = volume; @@ -348,8 +348,8 @@ public Builder setForceUpdateContainerCacheFromSCM(boolean value) { return this; } - public Builder setOverwriteGeneration(long generation) { - this.overwriteGeneration = generation; + public Builder setRewriteGeneration(long generation) { + this.rewriteGeneration = generation; return this; } diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java index 05eb82393b03..84a75d09e5b0 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmKeyInfo.java @@ -102,13 +102,13 @@ public static Codec getCodec(boolean ignorePipeline) { */ private final CopyOnWriteArrayList acls; - // OverwriteGeneration, when used in key creation indicates that a + // rewriteGeneration, when used in key creation indicates that a // key with the same keyName should exist with the given generation. // For a key commit to succeed, the original key should still be present with the // generation unchanged. // This allows a key to be created an committed atomically if the original has not // been modified. - private Long overwriteGeneration = null; + private Long rewriteGeneration = null; private OmKeyInfo(Builder b) { super(b); @@ -126,7 +126,7 @@ private OmKeyInfo(Builder b) { this.fileName = b.fileName; this.isFile = b.isFile; this.ownerName = b.ownerName; - this.overwriteGeneration = b.overwriteGeneration; + this.rewriteGeneration = b.rewriteGeneration; } public String getVolumeName() { @@ -169,12 +169,12 @@ public String getFileName() { return fileName; } - public void setOverwriteGeneration(Long generation) { - this.overwriteGeneration = generation; + public void setRewriteGeneration(Long generation) { + this.rewriteGeneration = generation; } - public Long getOverwriteGeneration() { - return overwriteGeneration; + public Long getRewriteGeneration() { + return rewriteGeneration; } public String getOwnerName() { @@ -460,7 +460,7 @@ public static class Builder extends WithParentObjectId.Builder { private FileChecksum fileChecksum; private boolean isFile; - private Long overwriteGeneration = null; + private Long rewriteGeneration = null; public Builder() { } @@ -589,8 +589,8 @@ public Builder setFile(boolean isAFile) { return this; } - public Builder setOverwriteGeneration(Long existingGeneration) { - this.overwriteGeneration = existingGeneration; + public Builder setRewriteGeneration(Long existingGeneration) { + this.rewriteGeneration = existingGeneration; return this; } @@ -698,8 +698,8 @@ private KeyInfo getProtobuf(boolean ignorePipeline, String fullKeyName, kb.setFileEncryptionInfo(OMPBHelper.convert(encInfo)); } kb.setIsFile(isFile); - if (overwriteGeneration != null) { - kb.setOverwriteGeneration(overwriteGeneration); + if (rewriteGeneration != null) { + kb.setRewriteGeneration(rewriteGeneration); } if (ownerName != null) { kb.setOwnerName(ownerName); @@ -750,8 +750,8 @@ public static OmKeyInfo getFromProtobuf(KeyInfo keyInfo) throws IOException { if (keyInfo.hasIsFile()) { builder.setFile(keyInfo.getIsFile()); } - if (keyInfo.hasOverwriteGeneration()) { - builder.setOverwriteGeneration(keyInfo.getOverwriteGeneration()); + if (keyInfo.hasRewriteGeneration()) { + builder.setRewriteGeneration(keyInfo.getRewriteGeneration()); } if (keyInfo.hasOwnerName()) { @@ -867,8 +867,8 @@ public OmKeyInfo copyObject() { if (fileChecksum != null) { builder.setFileChecksum(fileChecksum); } - if (overwriteGeneration != null) { - builder.setOverwriteGeneration(overwriteGeneration); + if (rewriteGeneration != null) { + builder.setRewriteGeneration(rewriteGeneration); } return builder.build(); diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java index 8815fe508e2d..a62898411d16 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java @@ -726,8 +726,8 @@ public OpenKeySession openKey(OmKeyArgs args) throws IOException { keyArgs.setSortDatanodes(args.getSortDatanodes()); - if (args.getOverwriteGeneration() != null) { - keyArgs.setOverwriteGeneration(args.getOverwriteGeneration()); + if (args.getRewriteGeneration() != null) { + keyArgs.setRewriteGeneration(args.getRewriteGeneration()); } req.setKeyArgs(keyArgs.build()); diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java index 1c7f201c264e..852d49871c49 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/helpers/TestOmKeyInfo.java @@ -67,7 +67,7 @@ public void protobufConversion() throws IOException { assertFalse(key.isHsync()); key.getMetadata().put(OzoneConsts.HSYNC_CLIENT_ID, "clientid"); assertTrue(key.isHsync()); - assertEquals(5678L, key.getOverwriteGeneration()); + assertEquals(5678L, key.getRewriteGeneration()); } @Test @@ -124,7 +124,7 @@ private OmKeyInfo createOmKeyInfo(ReplicationConfig replicationConfig) { .setReplicationConfig(replicationConfig) .addMetadata("key1", "value1") .addMetadata("key2", "value2") - .setOverwriteGeneration(5678L) + .setRewriteGeneration(5678L) .build(); } diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java index 90b645605e27..e3b0c4a7a717 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java @@ -1098,12 +1098,12 @@ public void testPutKey() throws IOException { } @Test - public void testOverwriteKey() throws IOException { + public void testRewriteKey() throws IOException { String volumeName = UUID.randomUUID().toString(); String bucketName = UUID.randomUUID().toString(); String value = "sample value"; - String overwriteValue = "overwrite value"; + String rewriteValue = "rewrite value"; store.createVolume(volumeName); OzoneVolume volume = store.getVolume(volumeName); @@ -1125,24 +1125,24 @@ public void testOverwriteKey() throws IOException { try (OzoneOutputStream out = bucket.rewriteKey(keyDetails.getName(), keyDetails.getDataSize(), keyDetails.getGeneration(), RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE), keyDetails.getMetadata())) { - out.write(overwriteValue.getBytes(UTF_8)); + out.write(rewriteValue.getBytes(UTF_8)); } try (OzoneInputStream is = bucket.readKey(keyName)) { - byte[] fileContent = new byte[overwriteValue.getBytes(UTF_8).length]; + byte[] fileContent = new byte[rewriteValue.getBytes(UTF_8).length]; is.read(fileContent); - assertEquals(overwriteValue, new String(fileContent, UTF_8)); + assertEquals(rewriteValue, new String(fileContent, UTF_8)); } // Delete the key bucket.deleteKey(keyName); - // Now try the overwrite again, and it should fail as the originally read key is no longer there. + // Now try the rewrite again, and it should fail as the originally read key is no longer there. assertThrows(IOException.class, () -> { try (OzoneOutputStream out = bucket.rewriteKey(keyDetails.getName(), keyDetails.getDataSize(), keyDetails.getGeneration(), RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE), keyDetails.getMetadata())) { - out.write(overwriteValue.getBytes(UTF_8)); + out.write(rewriteValue.getBytes(UTF_8)); } }); } diff --git a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto index bfe0a9e3c7dd..75a2562ae715 100644 --- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto +++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto @@ -1026,13 +1026,13 @@ message KeyArgs { // Force OM to update container cache location from SCL optional bool forceUpdateContainerCacheFromSCM = 20; optional string ownerName = 21; - // overwriteGeneration, when used in key creation indicates that a + // rewriteGeneration, when used in key creation indicates that a // key with the same keyName should exist with the given generation. // For a key commit to succeed, the original key should still be present with the // generation unchanged. // This allows a key to be created an committed atomically if the original has not // been modified. - optional uint64 overwriteGeneration = 22; + optional uint64 rewriteGeneration = 22; } message KeyLocation { @@ -1116,13 +1116,13 @@ message KeyInfo { optional FileChecksumProto fileChecksum = 18; optional bool isFile = 19; optional string ownerName = 20; - // overwriteGeneration, when used in key creation indicates that a + // rewriteGeneration, when used in key creation indicates that a // key with the same keyName should exist with the given generation. // For a key commit to succeed, the original key should still be present with the // generation unchanged. // This allows a key to be created an committed atomically if the original has not // been modified. - optional uint64 overwriteGeneration = 21; + optional uint64 rewriteGeneration = 21; } message BasicKeyInfo { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java index 75838eb4a962..910bb2f4316b 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/RequestAuditor.java @@ -82,9 +82,9 @@ default Map buildKeyArgsAuditMap(KeyArgs keyArgs) { auditMap.put(OzoneConsts.REPLICATION_CONFIG, ECReplicationConfig.toString(keyArgs.getEcReplicationConfig())); } - if (keyArgs.hasOverwriteGeneration()) { - auditMap.put(OzoneConsts.OVERWRITE_GENERATION, - String.valueOf(keyArgs.getOverwriteGeneration())); + if (keyArgs.hasRewriteGeneration()) { + auditMap.put(OzoneConsts.REWRITE_GENERATION, + String.valueOf(keyArgs.getRewriteGeneration())); } for (HddsProtos.KeyValue item : keyArgs.getMetadataList()) { if (ETAG.equals(item.getKey())) { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java index b06177ecffc0..48effbee4c2b 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java @@ -241,10 +241,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn " entry is not found in the OpenKey table", KEY_NOT_FOUND); } - validateAtomicOverwrite(keyToDelete, omKeyInfo, auditMap); - // Optimistic locking validation has passed. Now set the overwrite fields to null so they are + validateAtomicRewrite(keyToDelete, omKeyInfo, auditMap); + // Optimistic locking validation has passed. Now set the rewrite fields to null so they are // not persisted in the key table. - omKeyInfo.setOverwriteGeneration(null); + omKeyInfo.setRewriteGeneration(null); omKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( commitKeyArgs.getMetadataList())); @@ -504,18 +504,18 @@ public static OMRequest disallowHsync( return req; } - private void validateAtomicOverwrite(OmKeyInfo existing, OmKeyInfo toCommit, Map auditMap) + private void validateAtomicRewrite(OmKeyInfo existing, OmKeyInfo toCommit, Map auditMap) throws OMException { - if (toCommit.getOverwriteGeneration() != null) { + if (toCommit.getRewriteGeneration() != null) { // These values are not passed in the request keyArgs, so add them into the auditMap if they are present // in the open key entry. - auditMap.put(OzoneConsts.OVERWRITE_GENERATION, String.valueOf(toCommit.getOverwriteGeneration())); + auditMap.put(OzoneConsts.REWRITE_GENERATION, String.valueOf(toCommit.getRewriteGeneration())); if (existing == null) { - throw new OMException("Atomic overwrite is not allowed for a new key", KEY_NOT_FOUND); + throw new OMException("Atomic rewrite is not allowed for a new key", KEY_NOT_FOUND); } - if (!toCommit.getOverwriteGeneration().equals(existing.getUpdateID())) { + if (!toCommit.getRewriteGeneration().equals(existing.getUpdateID())) { throw new OMException("Cannot commit as current generation (" + existing.getUpdateID() + - ") does not match with the overwrite generation (" + toCommit.getOverwriteGeneration() + ")", + ") does not match with the rewrite generation (" + toCommit.getRewriteGeneration() + ")", KEY_NOT_FOUND); } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java index 26179644f038..4ede37e6b434 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java @@ -231,7 +231,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn keyName); OmKeyInfo dbKeyInfo = omMetadataManager.getKeyTable(getBucketLayout()) .getIfExist(dbKeyName); - validateAtomicOverwrite(dbKeyInfo, keyArgs); + validateAtomicRewrite(dbKeyInfo, keyArgs); OmBucketInfo bucketInfo = getBucketInfo(omMetadataManager, volumeName, bucketName); @@ -442,15 +442,15 @@ public static OMRequest blockCreateKeyWithBucketLayoutFromOldClient( return req; } - private void validateAtomicOverwrite(OmKeyInfo dbKeyInfo, KeyArgs keyArgs) + private void validateAtomicRewrite(OmKeyInfo dbKeyInfo, KeyArgs keyArgs) throws OMException { - if (keyArgs.hasOverwriteGeneration()) { + if (keyArgs.hasRewriteGeneration()) { // If a key does not exist, or if it exists but the updateID do not match, then fail this request. if (dbKeyInfo == null) { - throw new OMException("Key not found during expected overwrite", OMException.ResultCodes.KEY_NOT_FOUND); + throw new OMException("Key not found during expected rewrite", OMException.ResultCodes.KEY_NOT_FOUND); } - if (dbKeyInfo.getUpdateID() != keyArgs.getOverwriteGeneration()) { - throw new OMException("Generation mismatch during expected overwrite", OMException.ResultCodes.KEY_NOT_FOUND); + if (dbKeyInfo.getUpdateID() != keyArgs.getRewriteGeneration()) { + throw new OMException("Generation mismatch during expected rewrite", OMException.ResultCodes.KEY_NOT_FOUND); } } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java index 7d6290ba1aab..a29e63466e08 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java @@ -779,8 +779,8 @@ protected OmKeyInfo prepareFileInfo( dbKeyInfo.getMetadata().putAll(KeyValueUtil.getFromProtobuf( keyArgs.getMetadataList())); - if (keyArgs.hasOverwriteGeneration()) { - dbKeyInfo.setOverwriteGeneration(keyArgs.getOverwriteGeneration()); + if (keyArgs.hasRewriteGeneration()) { + dbKeyInfo.setRewriteGeneration(keyArgs.getRewriteGeneration()); } dbKeyInfo.setFileEncryptionInfo(encInfo); return dbKeyInfo; diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java index 0b1bebb9ce28..2fc38cbd3aa9 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java @@ -249,7 +249,7 @@ public void testAtomicRewrite() throws Exception { OmKeyInfo.Builder omKeyInfoBuilder = OMRequestTestUtils.createOmKeyInfo( volumeName, bucketName, keyName, replicationConfig, new OmKeyLocationInfoGroup(version, new ArrayList<>())); - omKeyInfoBuilder.setOverwriteGeneration(1L); + omKeyInfoBuilder.setRewriteGeneration(1L); OmKeyInfo omKeyInfo = omKeyInfoBuilder.build(); omKeyInfo.appendNewBlocks(allocatedLocationList, false); List acls = Collections.singletonList(OzoneAcl.parseAcl("user:foo:rw")); @@ -261,18 +261,18 @@ public void testAtomicRewrite() throws Exception { OmKeyInfo openKeyInfo = openKeyTable.get(openKey); assertNotNull(openKeyInfo); assertEquals(acls, openKeyInfo.getAcls()); - // At this stage, we have an openKey, with overwrite generation of 1. + // At this stage, we have an openKey, with rewrite generation of 1. // However there is no closed key entry, so the commit should fail. OMClientResponse omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); assertEquals(KEY_NOT_FOUND, omClientResponse.getOMResponse().getStatus()); // Now add the key to the key table, and try again, but with different generation - omKeyInfoBuilder.setOverwriteGeneration(null); + omKeyInfoBuilder.setRewriteGeneration(null); omKeyInfoBuilder.setUpdateID(0L); OmKeyInfo invalidKeyInfo = omKeyInfoBuilder.build(); closedKeyTable.put(getOzonePathKey(), invalidKeyInfo); - // This should fail as the updateID ia zero and the open key has overwrite generation of 1. + // This should fail as the updateID ia zero and the open key has rewrite generation of 1. omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); assertEquals(KEY_NOT_FOUND, omClientResponse.getOMResponse().getStatus()); @@ -282,12 +282,12 @@ public void testAtomicRewrite() throws Exception { closedKeyTable.delete(getOzonePathKey()); closedKeyTable.put(getOzonePathKey(), closedKeyInfo); - // Now the key should commit as the updateID and overwriteGeneration match. + // Now the key should commit as the updateID and rewrite Generation match. omClientResponse = omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L); assertEquals(OK, omClientResponse.getOMResponse().getStatus()); OmKeyInfo committedKey = closedKeyTable.get(getOzonePathKey()); - assertNull(committedKey.getOverwriteGeneration()); + assertNull(committedKey.getRewriteGeneration()); // Generation should be changed assertNotEquals(closedKeyInfo.getGeneration(), committedKey.getGeneration()); assertEquals(acls, committedKey.getAcls()); diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java index 0302bfecb759..4c7b2aa047da 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java @@ -706,14 +706,14 @@ protected OMRequest createKeyRequest(boolean isMultipartKey, int partNumber, private OMRequest createKeyRequest( boolean isMultipartKey, int partNumber, long keyLength, - ReplicationConfig repConfig, Long overwriteGeneration) { + ReplicationConfig repConfig, Long rewriteGeneration) { return createKeyRequest(isMultipartKey, partNumber, keyLength, repConfig, - overwriteGeneration, null); + rewriteGeneration, null); } private OMRequest createKeyRequest( boolean isMultipartKey, int partNumber, long keyLength, - ReplicationConfig repConfig, Long overwriteGeneration, Map metaData) { + ReplicationConfig repConfig, Long rewriteGeneration, Map metaData) { KeyArgs.Builder keyArgs = KeyArgs.newBuilder() .setVolumeName(volumeName).setBucketName(bucketName) @@ -732,8 +732,8 @@ private OMRequest createKeyRequest( if (isMultipartKey) { keyArgs.setMultipartNumber(partNumber); } - if (overwriteGeneration != null) { - keyArgs.setOverwriteGeneration(overwriteGeneration); + if (rewriteGeneration != null) { + keyArgs.setRewriteGeneration(rewriteGeneration); } if (metaData != null) { metaData.forEach((key, value) -> keyArgs.addMetadata(KeyValue.newBuilder() @@ -742,7 +742,7 @@ private OMRequest createKeyRequest( .build())); } - OzoneManagerProtocolProtos.CreateKeyRequest createKeyRequest = + CreateKeyRequest createKeyRequest = CreateKeyRequest.newBuilder().setKeyArgs(keyArgs).build(); return OMRequest.newBuilder() @@ -941,7 +941,7 @@ public void testAtomicRewrite( .setBucketName(bucketName) .setBucketLayout(getBucketLayout())); - // First, create a key with the overwrite ID - this should fail as no key exists + // First, create a key with the rewrite ID - this should fail as no key exists OMRequest omRequest = createKeyRequest(false, 0, 100, RatisReplicationConfig.getInstance(THREE), 1L); omRequest = doPreExecute(omRequest); @@ -949,7 +949,7 @@ public void testAtomicRewrite( OMClientResponse response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); assertEquals(KEY_NOT_FOUND, response.getOMResponse().getStatus()); - // Now pre-create the key in the system so we can overwrite it. + // Now pre-create the key in the system so we can rewrite it. Map metadata = Collections.singletonMap("metakey", "metavalue"); Map reWriteMetadata = Collections.singletonMap("metakey", "rewriteMetavalue"); @@ -964,7 +964,7 @@ public void testAtomicRewrite( List existingAcls = existingKeyInfo.getAcls(); assertEquals(acls, existingAcls); - // Create a request with an generation which doesn't match the current key + // Create a request with a generation which doesn't match the current key omRequest = createKeyRequest(false, 0, 100, RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getGeneration() + 1, reWriteMetadata); omRequest = doPreExecute(omRequest); @@ -973,7 +973,7 @@ public void testAtomicRewrite( // Still fails, as the matching key is not present. assertEquals(KEY_NOT_FOUND, response.getOMResponse().getStatus()); - // Now create the key with the correct overwrite generation + // Now create the key with the correct rewrite generation omRequest = createKeyRequest(false, 0, 100, RatisReplicationConfig.getInstance(THREE), existingKeyInfo.getGeneration(), reWriteMetadata); omRequest = doPreExecute(omRequest); @@ -981,13 +981,13 @@ public void testAtomicRewrite( response = omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 105L); assertEquals(OK, response.getOMResponse().getStatus()); - // Ensure the overwriteGeneration is persisted in the open key table + // Ensure the rewriteGeneration is persisted in the open key table String openKey = omMetadataManager.getOpenKey(volumeName, bucketName, keyName, omRequest.getCreateKeyRequest().getClientID()); OmKeyInfo openKeyInfo = omMetadataManager.getOpenKeyTable(omKeyCreateRequest.getBucketLayout()).get(openKey); - assertEquals(existingKeyInfo.getGeneration(), openKeyInfo.getOverwriteGeneration()); - // Creation time should remain the same on overwrite. + assertEquals(existingKeyInfo.getGeneration(), openKeyInfo.getRewriteGeneration()); + // Creation time should remain the same on rewrite. assertEquals(existingKeyInfo.getCreationTime(), openKeyInfo.getCreationTime()); // Update ID should change assertNotEquals(existingKeyInfo.getGeneration(), openKeyInfo.getGeneration()); From a87e092519825a97a6f048450442060112cb26c0 Mon Sep 17 00:00:00 2001 From: S O'Donnell Date: Thu, 9 May 2024 21:15:49 +0100 Subject: [PATCH 26/26] Remove generation from OzoneKey --- .../hadoop/ozone/client/OzoneBucket.java | 2 +- .../apache/hadoop/ozone/client/OzoneKey.java | 35 ++----------------- .../hadoop/ozone/client/OzoneKeyDetails.java | 13 ++++++- .../hadoop/ozone/client/OzoneBucketStub.java | 5 ++- 4 files changed, 18 insertions(+), 37 deletions(-) diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java index c36976fe5e63..012f029f51cc 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java @@ -1806,7 +1806,7 @@ private void addKeyPrefixInfoToResultList(String keyPrefix, keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), keyInfo.getReplicationConfig(), - keyInfo.isFile(), keyInfo.getOwnerName(), keyInfo.getUpdateID()); + keyInfo.isFile(), keyInfo.getOwnerName()); keysResultList.add(ozoneKey); } } diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java index 490986959c37..2d32a7272061 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKey.java @@ -70,17 +70,11 @@ public class OzoneKey { */ private final boolean isFile; - /** - * The generation of an existing key. This can be used with atomic commits, to - * ensure the key has not changed since the key details were read. - */ - private final Long generation; - @SuppressWarnings("parameternumber") public OzoneKey(String volumeName, String bucketName, String keyName, long size, long creationTime, long modificationTime, ReplicationConfig replicationConfig, - boolean isFile, String owner, Long generation) { + boolean isFile, String owner) { this.volumeName = volumeName; this.bucketName = bucketName; this.name = keyName; @@ -90,16 +84,6 @@ public OzoneKey(String volumeName, String bucketName, this.replicationConfig = replicationConfig; this.isFile = isFile; this.owner = owner; - this.generation = generation; - } - - @SuppressWarnings("parameternumber") - public OzoneKey(String volumeName, String bucketName, - String keyName, long size, long creationTime, - long modificationTime, ReplicationConfig replicationConfig, - boolean isFile, String owner) { - this(volumeName, bucketName, keyName, size, creationTime, modificationTime, replicationConfig, - isFile, owner, null); } @SuppressWarnings("parameternumber") @@ -108,19 +92,10 @@ public OzoneKey(String volumeName, String bucketName, long modificationTime, ReplicationConfig replicationConfig, Map metadata, boolean isFile, String owner) { this(volumeName, bucketName, keyName, size, creationTime, - modificationTime, replicationConfig, isFile, owner, null); + modificationTime, replicationConfig, isFile, owner); this.metadata.putAll(metadata); } - @SuppressWarnings("parameternumber") - public OzoneKey(String volumeName, String bucketName, - String keyName, long size, long creationTime, - long modificationTime, ReplicationConfig replicationConfig, - Map metadata, boolean isFile, String owner, Long generation) { - this(volumeName, bucketName, keyName, size, creationTime, - modificationTime, replicationConfig, isFile, owner, generation); - this.metadata.putAll(metadata); - } /** * Returns Volume Name associated with the Key. * @@ -214,10 +189,6 @@ public ReplicationConfig getReplicationConfig() { return replicationConfig; } - public Long getGeneration() { - return generation; - } - /** * Returns indicator if key is a file. * @return file @@ -234,7 +205,7 @@ public static OzoneKey fromKeyInfo(OmKeyInfo keyInfo) { return new OzoneKey(keyInfo.getVolumeName(), keyInfo.getBucketName(), keyInfo.getKeyName(), keyInfo.getDataSize(), keyInfo.getCreationTime(), keyInfo.getModificationTime(), keyInfo.getReplicationConfig(), - keyInfo.getMetadata(), keyInfo.isFile(), keyInfo.getOwnerName(), keyInfo.getGeneration()); + keyInfo.getMetadata(), keyInfo.isFile(), keyInfo.getOwnerName()); } } diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java index 30e697d8fd23..cd2978fce189 100644 --- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java +++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneKeyDetails.java @@ -42,6 +42,12 @@ public class OzoneKeyDetails extends OzoneKey { private final CheckedSupplier contentSupplier; + /** + * The generation of an existing key. This can be used with atomic commits, to + * ensure the key has not changed since the key details were read. + */ + private final Long generation; + /** * Constructs OzoneKeyDetails from OmKeyInfo. */ @@ -55,10 +61,11 @@ public OzoneKeyDetails(String volumeName, String bucketName, String keyName, CheckedSupplier contentSupplier, boolean isFile, String owner, Long generation) { super(volumeName, bucketName, keyName, size, creationTime, - modificationTime, replicationConfig, metadata, isFile, owner, generation); + modificationTime, replicationConfig, metadata, isFile, owner); this.ozoneKeyLocations = ozoneKeyLocations; this.feInfo = feInfo; this.contentSupplier = contentSupplier; + this.generation = generation; } /** @@ -89,6 +96,10 @@ public FileEncryptionInfo getFileEncryptionInfo() { return feInfo; } + public Long getGeneration() { + return generation; + } + /** * Get OzoneInputStream to read the content of the key. * @return OzoneInputStream diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java index d4993f4f3f90..0dc548867676 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneBucketStub.java @@ -313,8 +313,7 @@ public OzoneKey headObject(String key) throws IOException { ozoneKeyDetails.getReplicationConfig(), ozoneKeyDetails.getMetadata(), ozoneKeyDetails.isFile(), - ozoneKeyDetails.getOwner(), - ozoneKeyDetails.getGeneration()); + ozoneKeyDetails.getOwner()); } else { throw new OMException(ResultCodes.KEY_NOT_FOUND); } @@ -369,7 +368,7 @@ public Iterator listKeys(String keyPrefix, key.getDataSize(), key.getCreationTime().getEpochSecond() * 1000, key.getModificationTime().getEpochSecond() * 1000, - key.getReplicationConfig(), key.isFile(), key.getOwner(), key.getGeneration()); + key.getReplicationConfig(), key.isFile(), key.getOwner()); }).collect(Collectors.toList()); if (prevKey != null) {