Skip to content

HDDS-13919. S3 Conditional Writes (PutObject) [2/2] - Reuse Atomic Rewrite at Commit Path #10023

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
peterxcli merged 2 commits into from
Apr 3, 2026
Merged

HDDS-13919. S3 Conditional Writes (PutObject) [2/2] - Reuse Atomic Rewrite at Commit Path #10023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f04c23
HDDS-13919. S3 Conditional Writes (PutObject)
peterxcli Apr 1, 2026
019991a
fix test
peterxcli Apr 1, 2026
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
18 changes: 0 additions & 18 deletions ...zone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,24 +642,6 @@ protected void validateAtomicRewrite(OmKeyInfo existing, OmKeyInfo toCommit, Map
}
}
}

if (toCommit.getExpectedETag() != null) {
String expectedETag = toCommit.getExpectedETag();
auditMap.put("expectedETag", expectedETag);

if (existing == null) {
throw new OMException("Key not found for If-Match at commit",
OMException.ResultCodes.KEY_NOT_FOUND);
}
if (!existing.hasEtag()) {
throw new OMException("Key does not have an ETag at commit",
OMException.ResultCodes.ETAG_NOT_AVAILABLE);
}
if (!existing.isEtagEquals(expectedETag)) {
throw new OMException("ETag changed during write (concurrent modification)",
OMException.ResultCodes.ETAG_MISMATCH);
}
}
}

}
43 changes: 29 additions & 14 deletions ...zone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
OmKeyInfo dbKeyInfo = omMetadataManager.getKeyTable(getBucketLayout())
.getIfExist(dbKeyName);
validateAtomicRewrite(dbKeyInfo, keyArgs);
keyArgs = validateAndRewriteIfMatchAsExpectedGeneration(keyArgs, dbKeyInfo);

OmBucketInfo bucketInfo =
getBucketInfo(omMetadataManager, volumeName, bucketName);
Expand Down Expand Up @@ -497,20 +498,34 @@ protected void validateAtomicRewrite(OmKeyInfo dbKeyInfo, KeyArgs keyArgs)
}
}

if (keyArgs.hasExpectedETag()) {
String expectedETag = keyArgs.getExpectedETag();
if (dbKeyInfo == null) {
throw new OMException("Key not found for If-Match",
OMException.ResultCodes.KEY_NOT_FOUND);
}
if (!dbKeyInfo.hasEtag()) {
throw new OMException("Key does not have an ETag",
OMException.ResultCodes.ETAG_NOT_AVAILABLE);
}
if (!dbKeyInfo.isEtagEquals(expectedETag)) {
throw new OMException("ETag mismatch",
OMException.ResultCodes.ETAG_MISMATCH);
}
}
Copy link
Copy Markdown

@yandrey321 yandrey321 Apr 1, 2026

Choose a reason for hiding this comment

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

it seems like the same logic ad in CommitRequest

Copy link
Copy Markdown
Member Author

@peterxcli peterxcli Apr 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

correct, and that's actually unnecessary as the reason I wrote in the description.

  1. Extract Generation: If ETag matches, extract existingKey.updateID.
  2. Create Open Key: Create open key entry with expectedDataGeneration = existingKey.updateID.

Copy link
Copy Markdown
Contributor

@ivandika3 ivandika3 Apr 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Ok after thinking a bit more, I think using expectedDataGeneration ensure that only one PutObject can succeed even if two PutObject are using the same valid If-Match.

From the if-match semantic, if there are two concurrent clients that are calling PutObject with the same if-match, both should succeed since the ETag is the same although expectedDataGeneration are not the same.

So in the end, our implementation of if-match is more strict (not necessarily a bad thing). However, it might cause duplicated requests to be rejected. Let me know what you think.

Copy link
Copy Markdown
Member Author

@peterxcli peterxcli Apr 3, 2026

Choose a reason for hiding this comment

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

From the if-match semantic, if there are two concurrent clients that are calling PutObject with the same if-match, both should succeed since the ETag is the same although expectedDataGeneration are not the same.

This would only be true if the ETag of the object didn't change upon a successful (Conditional) PutObject.

Copy link
Copy Markdown
Contributor

@ivandika3 ivandika3 Apr 3, 2026

Choose a reason for hiding this comment

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

Yes, overall I'm OK with the current approach.

Copy link
Copy Markdown
Member Author

@peterxcli peterxcli Apr 3, 2026

Choose a reason for hiding this comment

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

However, it might cause duplicated requests to be rejected

let me think about which applications built on top of the AWS S3 spec would be affected by this stricter semantics.

Copy link
Copy Markdown
Member Author

@peterxcli peterxcli Apr 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

@ivandika3 The only thing I can think of is that the AWS behavior can suffer from the ABA problem if duplicate conditional PutObject requests are not rejected. In contrast, our version behaves more like a tagged pointer, which prevents the ABA scenario. So everything else stays the same; only concurrent duplicate or ABA-style requests gain a stronger safety guarantee.

Copy link
Copy Markdown
Contributor

@ivandika3 ivandika3 Apr 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Good point on the ABA problem although it might be fine since the latest object is still back to the original content (i.e. A).

My current worry if a single client sends a PutObject, but retries it again (e.g. due to some network restriction, etc), the retry might fail. We can try to look into when checking our AWS S3 spec compatibility. Saw your https://github.com/peterxcli/ozone-s3-compatibility, great initiative!

Copy link
Copy Markdown
Member Author

@peterxcli peterxcli Apr 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

My current worry is that if a single client sends a PutObject but retries it again (e.g., due to some network restriction, etc.), the retry might fail. We can try to look into this when checking our AWS S3 spec compatibility.

Thanks @ivandika3, this prompted me to review the conditional put requests in the AWS spec, which mentions that both If-Match and If-None-Match requests can throw two types of exceptions: PreconditionFailed (412) and ConditionalRequestConflict (409). Applying this to our case, PreconditionFailed should be thrown if the validation is violated when running createKeyRequest, whereas ConditionalRequestConflict should be thrown during commitKeyRequest.

I've raised a Jira ticket for this. BTW, the PR is also ready:

My current worry is that if a single client sends a PutObject but retries it again (e.g., due to some network restriction, etc.), the retry might fail.

I think upon a client retry, the execution goes through the createKey -> commitKey flow again. This stores the latest updateID of the key in the openKeyTable as expectedDataGeneration. Then, when it comes to the atomic rewrite key commit, the stored expectedDataGeneration matches the current updateID of the key in the keyTable.

So in conclusion, concurrent duplicate conditional requests might result in one of them failing, but if the duplicate requests are dependent on each other (i.e., causal), then it won't be a problem.

Copy link
Copy Markdown
Member Author

@peterxcli peterxcli Apr 5, 2026

Choose a reason for hiding this comment

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

Saw your [peterxcli/ozone-s3-compatibility](https://github.com/peterxcli/ozone-s3-compatibility?rgh-link-date=2026-04-03T12%3A56%3A30Z), great initiative!

Thanks @ivandika3 for recognizing the work!

I've noticed that Ceph's s3-tests might not be entirely accurate for true S3 compatibility, as some nuances still exist. It seems they mark certain tests with a fails_on_aws flag, which might be because they implemented the AWS S3 spec based on their own interpretation. What's worse is that they didn't do this consistently for all cases.

Take an example related to conditional requests: they send an If-None-Match request where the ETag is not an asterisk, whereas the AWS S3 spec explicitly states: "Expects the * character (asterisk)." 

So, an additional review of that S3 compatibility suite is required; the actual compatibility numbers might not be as bad as we seem.


protected KeyArgs validateAndRewriteIfMatchAsExpectedGeneration(
KeyArgs keyArgs, OmKeyInfo dbKeyInfo) throws OMException {
if (!keyArgs.hasExpectedETag()) {
return keyArgs;
}

String expectedETag = keyArgs.getExpectedETag();
if (dbKeyInfo == null) {
throw new OMException("Key not found for If-Match",
OMException.ResultCodes.KEY_NOT_FOUND);
}
if (!dbKeyInfo.hasEtag()) {
throw new OMException("Key does not have an ETag",
OMException.ResultCodes.ETAG_NOT_AVAILABLE);
}
if (!dbKeyInfo.isEtagEquals(expectedETag)) {
throw new OMException("ETag mismatch",
OMException.ResultCodes.ETAG_MISMATCH);
}
if (keyArgs.hasExpectedDataGeneration()) {
return keyArgs;
}

return keyArgs.toBuilder()
.setExpectedDataGeneration(dbKeyInfo.getUpdateID())
.clearExpectedETag()
.build();
}
}
2 changes: 2 additions & 0 deletions ...nager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequestWithFSO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, Execut
omMetadataManager, dbFileKey, keyName);
}
validateAtomicRewrite(dbFileInfo, keyArgs);
keyArgs = validateAndRewriteIfMatchAsExpectedGeneration(
keyArgs, dbFileInfo);

// Check if a file or directory exists with same key name.
if (pathInfoFSO.getDirectoryResult() == DIRECTORY_EXISTS) {
Expand Down
137 changes: 0 additions & 137 deletions ...-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCommitRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,143 +350,6 @@ public void testAtomicCreateIfNotExistsCommitKeyAlreadyExists() throws Exception
assertEquals(KEY_ALREADY_EXISTS, omClientResponse.getOMResponse().getStatus());
}

@Test
public void testCommitWithExpectedETagSuccess() 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());

String expectedETag = "matching-etag";
OmKeyInfo.Builder omKeyInfoBuilder = OMRequestTestUtils.createOmKeyInfo(
volumeName, bucketName, keyName, replicationConfig,
new OmKeyLocationInfoGroup(version, new ArrayList<>()));
omKeyInfoBuilder.setExpectedETag(expectedETag);

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

// Add closed key with matching ETag
OmKeyInfo closedKeyInfo = OMRequestTestUtils.createOmKeyInfo(
volumeName, bucketName, keyName, replicationConfig,
new OmKeyLocationInfoGroup(version, new ArrayList<>()))
.addMetadata(OzoneConsts.ETAG, expectedETag).build();
closedKeyTable.put(getOzonePathKey(), closedKeyInfo);

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

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

@Test
public void testCommitWithExpectedETagMismatch() 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.setExpectedETag("expected-etag");

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

// Add closed key with different ETag
OmKeyInfo closedKeyInfo = OMRequestTestUtils.createOmKeyInfo(
volumeName, bucketName, keyName, replicationConfig,
new OmKeyLocationInfoGroup(version, new ArrayList<>()))
.addMetadata(OzoneConsts.ETAG, "different-etag").build();
closedKeyTable.put(getOzonePathKey(), closedKeyInfo);

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

@Test
public void testCommitWithExpectedETagNoETagOnKey() 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.setExpectedETag("expected-etag");

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

// Add closed key WITHOUT ETag
OmKeyInfo closedKeyInfo = OMRequestTestUtils.createOmKeyInfo(
volumeName, bucketName, keyName, replicationConfig,
new OmKeyLocationInfoGroup(version, new ArrayList<>())).build();
closedKeyTable.put(getOzonePathKey(), closedKeyInfo);

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

@Test
public void testValidateAndUpdateCacheWithUncommittedBlocks()
throws Exception {
Expand Down
6 changes: 4 additions & 2 deletions ...-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,11 +340,13 @@ public void testCreateWithExpectedETagSuccess(
omKeyCreateRequest.validateAndUpdateCache(ozoneManager, 100L);
assertEquals(OK, response.getOMResponse().getStatus());

// Verify open key was created with expectedETag
// Verify open key was normalized onto the atomic rewrite generation.
OmKeyInfo openKeyInfo = omMetadataManager.getOpenKeyTable(getBucketLayout())
.get(getOpenKey(id));
assertNotNull(openKeyInfo);
assertEquals(expectedETag, openKeyInfo.getExpectedETag());
assertEquals(existingKeyInfo.getUpdateID(),
openKeyInfo.getExpectedDataGeneration());
assertNull(openKeyInfo.getExpectedETag());
// Creation time should remain the same on rewrite
assertEquals(existingKeyInfo.getCreationTime(),
openKeyInfo.getCreationTime());
Expand Down