Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ public final class OzoneConsts {
public static final String TENANT = "tenant";
public static final String USER_PREFIX = "userPrefix";
public static final String REWRITE_GENERATION = "rewriteGeneration";
public static final long EXPECTED_GEN_CREATE_IF_NOT_EXISTS = -1L;
public static final String FROM_SNAPSHOT = "fromSnapshot";
public static final String TO_SNAPSHOT = "toSnapshot";
public static final String TOKEN = "token";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1029,8 +1029,8 @@ public List<OzoneFileStatus> listStatus(String keyName, boolean recursive,
*
* @param prefix Optional string to filter for the selected keys.
*/
public OzoneMultipartUploadList listMultipartUploads(String prefix,
String keyMarker, String uploadIdMarker, int maxUploads)
public OzoneMultipartUploadList listMultipartUploads(String prefix,
String keyMarker, String uploadIdMarker, int maxUploads)
throws IOException {
return proxy.listMultipartUploads(volumeName, getName(), prefix, keyMarker, uploadIdMarker, maxUploads);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,7 @@ public void createBucket(
builder.setDefaultReplicationConfig(defaultReplicationConfig);
}

String replicationType = defaultReplicationConfig == null
String replicationType = defaultReplicationConfig == null
? "server-side default replication type"
: defaultReplicationConfig.getType().toString();

Expand Down Expand Up @@ -1315,7 +1315,7 @@ public List<OzoneBucket> listBuckets(String volumeName, String bucketPrefix,
List<OmBucketInfo> buckets = ozoneManagerClient.listBuckets(
volumeName, prevBucket, bucketPrefix, maxListResult, hasSnapshot);

return buckets.stream().map(bucket ->
return buckets.stream().map(bucket ->
OzoneBucket.newBuilder(conf, this)
.setVolumeName(bucket.getVolumeName())
.setName(bucket.getBucketName())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ public enum ResultCodes {
USER_MISMATCH, // Error code when requested user name passed is different
// from remote user.

INVALID_PART, // When part name is not found or not matching with partname
INVALID_PART, // When part name is not found or not matching with partname
// in OM MPU partInfo.

INVALID_PART_ORDER, // When list of parts mentioned to complete MPU are not
Expand Down Expand Up @@ -267,7 +267,7 @@ public enum ResultCodes {
UNAUTHORIZED,

S3_SECRET_ALREADY_EXISTS,

INVALID_PATH,
TOO_MANY_BUCKETS,
KEY_UNDER_LEASE_RECOVERY,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,13 @@
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SCM_BLOCK_SIZE_DEFAULT;
import static org.apache.hadoop.ozone.OzoneConsts.DEFAULT_OM_UPDATE_ID;
import static org.apache.hadoop.ozone.OzoneConsts.ETAG;
import static org.apache.hadoop.ozone.OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS;
import static org.apache.hadoop.ozone.OzoneConsts.GB;
import static org.apache.hadoop.ozone.OzoneConsts.MD5_HASH;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
import static org.apache.hadoop.ozone.client.OzoneClientTestUtils.assertKeyContent;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_DIR_DELETING_SERVICE_INTERVAL;
import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_ALREADY_EXISTS;
import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_NOT_FOUND;
import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.NO_SUCH_MULTIPART_UPLOAD_ERROR;
import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.PARTIAL_RENAME;
Expand Down Expand Up @@ -1370,6 +1372,54 @@ void rewriteFailsDueToOutdatedGenerationAtCommit(BucketLayout layout) throws IOE
assertUnchanged(keyInfo, ozoneManager.lookupKey(keyArgs));
}

@ParameterizedTest
@EnumSource
void rewriteFailsWhenKeyExists(BucketLayout layout) throws IOException {
checkFeatureEnable(OzoneManagerVersion.ATOMIC_REWRITE_KEY);
OzoneBucket bucket = createBucket(layout);
OzoneKeyDetails key1Details = createTestKey(bucket, "key1", "value".getBytes(UTF_8));
OzoneOutputStream key2Out = openTestKey(bucket, "key2", "value");
OzoneOutputStream key3Out = openTestKey(bucket, "key3", "value");

// Test 1: Rewrite with -1 fails when key is already committed
OMException e = assertThrows(OMException.class, () -> {
bucket.rewriteKey(
key1Details.getName(),
key1Details.getDataSize(),
EXPECTED_GEN_CREATE_IF_NOT_EXISTS,
RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE),
key1Details.getMetadata());
});

assertEquals(KEY_ALREADY_EXISTS, e.getResult());
assertThat(e).hasMessageContaining("Key already exists");

// Test 2: Rewrite with -1 succeeds when key is open but not yet committed
assertDoesNotThrow(() -> {
bucket.rewriteKey("key2",
1024,
EXPECTED_GEN_CREATE_IF_NOT_EXISTS,
RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE),
singletonMap("key", "value"));
});
key2Out.close();

// Test 3: After rewrite completes, attempting to rewrite again with -1 fails
key3Out.write("value".getBytes(UTF_8));
key3Out.close();

e = assertThrows(OMException.class, () -> {
bucket.rewriteKey("key2",
1024,
EXPECTED_GEN_CREATE_IF_NOT_EXISTS,
RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE),
singletonMap("key", "value"));
});

assertEquals(KEY_ALREADY_EXISTS, e.getResult());
assertThat(e).hasMessageContaining("Key already exists");
}

@ParameterizedTest
@EnumSource
void cannotRewriteDeletedKey(BucketLayout layout) throws IOException {
Expand Down Expand Up @@ -4385,6 +4435,12 @@ private void completeMultipartUpload(OzoneBucket bucket, String keyName,
assertNotNull(omMultipartUploadCompleteInfo.getHash());
}

private OzoneOutputStream openTestKey(OzoneBucket bucket, String keyName, String keyValue) throws IOException {
return bucket.createKey(keyName, keyValue.getBytes(UTF_8).length,
RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE),
singletonMap("key", RandomStringUtils.secure().nextAscii(10)));
}

private OzoneKeyDetails createTestKey(OzoneBucket bucket) throws IOException {
return createTestKey(bucket, getTestName(), UUID.randomUUID().toString());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -616,14 +616,23 @@ protected void validateAtomicRewrite(OmKeyInfo existing, OmKeyInfo toCommit, Map
if (toCommit.getExpectedDataGeneration() != 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.REWRITE_GENERATION, String.valueOf(toCommit.getExpectedDataGeneration()));
if (existing == null) {
throw new OMException("Atomic rewrite is not allowed for a new key", KEY_NOT_FOUND);
}
if (!toCommit.getExpectedDataGeneration().equals(existing.getUpdateID())) {
throw new OMException("Cannot commit as current generation (" + existing.getUpdateID() +
") does not match the expected generation to rewrite (" + toCommit.getExpectedDataGeneration() + ")",
KEY_NOT_FOUND);
Long expectedGen = toCommit.getExpectedDataGeneration();
auditMap.put(OzoneConsts.REWRITE_GENERATION, String.valueOf(expectedGen));

if (expectedGen == OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS) {
if (existing != null) {
throw new OMException("Key already exists",
OMException.ResultCodes.KEY_ALREADY_EXISTS);
}
Comment on lines +623 to +626
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you help check whether it's better to use a new ResultCodes specific to return S3 412 Precondition Failed? Although KEY_ALREADY_EXISTS does not seem to be handled by S3G currently, it might be safer to add a new result code just in case KEY_ALREADY_EXISTS will be used for other S3 operations.

} else {
if (existing == null) {
throw new OMException("Atomic rewrite is not allowed for a new key", KEY_NOT_FOUND);
}
if (!toCommit.getExpectedDataGeneration().equals(existing.getUpdateID())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can reuse expectedGen, also applied to the following exception. expectedGen can be pushed before the if condition.

throw new OMException("Cannot commit as current generation (" + existing.getUpdateID() +
") does not match the expected generation to rewrite (" + toCommit.getExpectedDataGeneration() + ")",
KEY_NOT_FOUND);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.apache.hadoop.hdds.scm.container.common.helpers.ExcludeList;
import org.apache.hadoop.hdds.utils.UniqueId;
import org.apache.hadoop.ozone.OmUtils;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.OzoneManagerVersion;
import org.apache.hadoop.ozone.audit.OMAction;
import org.apache.hadoop.ozone.om.OMMetadataManager;
Expand Down Expand Up @@ -178,7 +179,7 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {

KeyArgs.Builder finalNewKeyArgs = newKeyArgs;
KeyArgs resolvedKeyArgs =
captureLatencyNs(perfMetrics.getCreateKeyResolveBucketAndAclCheckLatencyNs(),
captureLatencyNs(perfMetrics.getCreateKeyResolveBucketAndAclCheckLatencyNs(),
() -> resolveBucketAndCheckKeyAcls(finalNewKeyArgs.build(), ozoneManager,
IAccessAuthorizer.ACLType.CREATE));
newCreateKeyRequest =
Expand Down Expand Up @@ -358,7 +359,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
} else {
perfMetrics.addCreateKeyFailureLatencyNs(createKeyLatency);
}

if (acquireLock) {
mergeOmLockDetails(ozoneLockStrategy
.releaseWriteLock(omMetadataManager, volumeName,
Expand Down Expand Up @@ -460,12 +461,22 @@ public static OMRequest blockCreateKeyWithBucketLayoutFromOldClient(
protected void validateAtomicRewrite(OmKeyInfo dbKeyInfo, KeyArgs keyArgs)
throws OMException {
if (keyArgs.hasExpectedDataGeneration()) {
// 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 rewrite", OMException.ResultCodes.KEY_NOT_FOUND);
}
if (dbKeyInfo.getUpdateID() != keyArgs.getExpectedDataGeneration()) {
throw new OMException("Generation mismatch during expected rewrite", OMException.ResultCodes.KEY_NOT_FOUND);
long expectedGen = keyArgs.getExpectedDataGeneration();
// If expectedGen is EXPECTED_GEN_CREATE_IF_NOT_EXISTS, it means the key MUST NOT exist (If-None-Match)
if (expectedGen == OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS) {
if (dbKeyInfo != null) {
throw new OMException("Key already exists",
OMException.ResultCodes.KEY_ALREADY_EXISTS);
}
} else {
// 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 rewrite", OMException.ResultCodes.KEY_NOT_FOUND);
}
if (dbKeyInfo.getUpdateID() != expectedGen) {
throw new OMException("Generation mismatch during expected rewrite",
OMException.ResultCodes.KEY_NOT_FOUND);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.hadoop.ozone.om.request.key;

import static org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status.KEY_ALREADY_EXISTS;
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;
Expand Down Expand Up @@ -279,6 +280,76 @@ public void testAtomicRewrite() throws Exception {
assertEquals(acls, committedKey.getAcls());
}

@Test
public void testAtomicCreateIfNotExistsCommitKeyAbsent() throws Exception {
Table<String, OmKeyInfo> openKeyTable = omMetadataManager.getOpenKeyTable(getBucketLayout());
Table<String, OmKeyInfo> closedKeyTable = omMetadataManager.getKeyTable(getBucketLayout());

OMRequest modifiedOmRequest = doPreExecute(createCommitKeyRequest());
OMKeyCommitRequest omKeyCommitRequest = getOmKeyCommitRequest(modifiedOmRequest);
KeyArgs keyArgs = modifiedOmRequest.getCommitKeyRequest().getKeyArgs();

OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName,
omMetadataManager, omKeyCommitRequest.getBucketLayout());

List<OmKeyLocationInfo> 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.setExpectedDataGeneration(OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS);

String openKey = addKeyToOpenKeyTable(allocatedLocationList, omKeyInfoBuilder);
assertNotNull(openKeyTable.get(openKey));
assertNull(closedKeyTable.get(getOzonePathKey()));

OMClientResponse omClientResponse =
omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L);
assertEquals(OK, omClientResponse.getOMResponse().getStatus());

OmKeyInfo committedKey = closedKeyTable.get(getOzonePathKey());
assertNotNull(committedKey);
assertNull(committedKey.getExpectedDataGeneration());
}

@Test
public void testAtomicCreateIfNotExistsCommitKeyAlreadyExists() throws Exception {
Table<String, OmKeyInfo> openKeyTable = omMetadataManager.getOpenKeyTable(getBucketLayout());
Table<String, OmKeyInfo> closedKeyTable = omMetadataManager.getKeyTable(getBucketLayout());

OMRequest modifiedOmRequest = doPreExecute(createCommitKeyRequest());
OMKeyCommitRequest omKeyCommitRequest = getOmKeyCommitRequest(modifiedOmRequest);
KeyArgs keyArgs = modifiedOmRequest.getCommitKeyRequest().getKeyArgs();

OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName,
omMetadataManager, omKeyCommitRequest.getBucketLayout());

List<OmKeyLocationInfo> 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.setExpectedDataGeneration(OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS);

String openKey = addKeyToOpenKeyTable(allocatedLocationList, omKeyInfoBuilder);
assertNotNull(openKeyTable.get(openKey));

OmKeyInfo existingClosedKey = OMRequestTestUtils.createOmKeyInfo(
volumeName, bucketName, keyName, replicationConfig,
new OmKeyLocationInfoGroup(version, new ArrayList<>())).build();
closedKeyTable.put(getOzonePathKey(), existingClosedKey);

OMClientResponse omClientResponse =
omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L);
assertEquals(KEY_ALREADY_EXISTS, omClientResponse.getOMResponse().getStatus());
}

@Test
public void testValidateAndUpdateCacheWithUncommittedBlocks()
throws Exception {
Expand Down Expand Up @@ -456,7 +527,7 @@ private Map<String, RepeatedOmKeyInfo> doKeyCommit(boolean isHSync,
.collect(Collectors.toList());
String openKey = addKeyToOpenKeyTable(allocatedBlockList);
String ozoneKey = getOzonePathKey();

OMClientResponse omClientResponse =
omKeyCommitRequest.validateAndUpdateCache(ozoneManager, 100L);
assertEquals(OK,
Expand Down
Loading