You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "peterxcli (via GitHub)" <gi...@apache.org> on 2026/02/24 11:32:41 UTC
[PR] Hdds 13919 conditional writes [ozone]
peterxcli opened a new pull request, #9815:
URL: https://github.com/apache/ozone/pull/9815
## What changes were proposed in this pull request?
Should be merged after https://github.com/apache/ozone/pull/9332
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13919
## How was this patch tested?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r2999725804
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -187,8 +188,18 @@ public Response put(
throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);
} else if (ex.getResult() == ResultCodes.FILE_ALREADY_EXISTS) {
throw newError(S3ErrorTable.NO_OVERWRITE, keyPath, ex);
+ } else if (ex.getResult() == ResultCodes.KEY_ALREADY_EXISTS) {
+ throw newError(PRECOND_FAILED, keyPath, ex);
Review Comment:
I think the `ex` here we are using to create the new error already contain the error info from OM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #9815:
URL: https://github.com/apache/ozone/pull/9815#issuecomment-4170979373
Merged. Thanks @peterxcli and @ivandika3
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3020567161
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -271,6 +280,38 @@ Response handlePutRequest(ObjectRequestContext context, String keyPath, InputStr
return Response.ok().status(HttpStatus.SC_OK).build();
}
+ String ifNoneMatch = getHeaders().getHeaderString(
Review Comment:
I’m still thinking about how to refactor this(line 283-314), as the conditional request validation logic dominates the function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3012857605
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1435,6 +1435,68 @@ public OzoneOutputStream rewriteKey(String volumeName, String bucketName, String
return createOutputStream(openKey);
}
+ @Override
+ public OzoneOutputStream createKeyIfNotExists(String volumeName,
+ String bucketName, String keyName, long size,
+ ReplicationConfig replicationConfig, Map<String, String> metadata,
+ Map<String, String> tags) throws IOException {
+ if (omVersion.compareTo(OzoneManagerVersion.ATOMIC_REWRITE_KEY) < 0) {
+ throw new IOException(
+ "OzoneManager does not support atomic key creation.");
+ }
+
+ createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig);
+
+ OmKeyArgs.Builder builder = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(keyName)
+ .setDataSize(size)
+ .setReplicationConfig(replicationConfig)
+ .addAllMetadataGdpr(metadata)
+ .addAllTags(tags)
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setExpectedDataGeneration(
+ OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS);
+
+ OpenKeySession openKey = ozoneManagerClient.openKey(builder.build());
+ if (isS3GRequest.get() && size == 0) {
+ openKey.getKeyInfo().setDataSize(0);
+ }
+ return createOutputStream(openKey);
+ }
+
+ @Override
+ @SuppressWarnings("checkstyle:parameternumber")
+ public OzoneOutputStream rewriteKeyIfMatch(String volumeName,
Review Comment:
looks like we can refactor & combine rewriteKey() and rewriteKeyIfMatch()
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1435,6 +1435,68 @@ public OzoneOutputStream rewriteKey(String volumeName, String bucketName, String
return createOutputStream(openKey);
}
+ @Override
+ public OzoneOutputStream createKeyIfNotExists(String volumeName,
Review Comment:
this method looks very much the same as createKey(), except the addition of expectedDataGeneration field. We should refactor & merge them into the same method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3017185075
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1176,6 +1185,11 @@ message KeyInfo {
// This allows a key to be created an committed atomically if the original has not
// been modified.
optional uint64 expectedDataGeneration = 22;
+
+ // expectedETag, when set, indicates that the existing key must have
+ // the given ETag for the operation to succeed. This is used for
+ // S3 conditional writes with the If-Match header.
+ optional string expectedETag = 23;
Review Comment:
good point. let's refactor the `expectedDataGeneration` and `expectedETag` into another dedicated proto message as follow-up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3012836701
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,48 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName,
+ OzoneBucket bucket, String keyPath, long length,
+ ReplicationConfig replicationConfig, Map<String, String> customMetadata,
+ Map<String, String> tags, String ifNoneMatch, String ifMatch)
+ throws IOException {
+ if (ifNoneMatch != null && "*".equals(ifNoneMatch.trim())) {
+ return getClientProtocol().createKeyIfNotExists(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
Review Comment:
If-None-Match handling only recognizes an unquoted `*` and otherwise silently falls back to an unconditional createKey(). Per the design doc, clients may send `If-None-Match: "*"`; in that case this code would incorrectly allow overwrites. Consider normalizing/parsing the header value (similar to parseETag) and rejecting unsupported If-None-Match values instead of ignoring them.
```suggestion
if (ifNoneMatch != null) {
// Normalize the If-None-Match header value, accepting both quoted and
// unquoted "*" and rejecting any other value instead of silently
// falling back to an unconditional createKey().
String normalizedIfNoneMatch = parseETag(ifNoneMatch);
if ("*".equals(normalizedIfNoneMatch)) {
return getClientProtocol().createKeyIfNotExists(
volumeName, bucketName, keyPath, length, replicationConfig,
customMetadata, tags);
} else {
throw new OS3Exception(
PRECOND_FAILED,
"Unsupported If-None-Match header value",
HttpStatus.SC_PRECONDITION_FAILED);
}
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,48 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName,
+ OzoneBucket bucket, String keyPath, long length,
Review Comment:
The openKeyForPut(...) helper takes an OzoneBucket parameter but does not use it. Removing the unused parameter will simplify the signature and avoid confusion about whether bucket-specific info is required for conditional puts.
```suggestion
String keyPath, long length,
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,48 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName,
+ OzoneBucket bucket, String keyPath, long length,
+ ReplicationConfig replicationConfig, Map<String, String> customMetadata,
+ Map<String, String> tags, String ifNoneMatch, String ifMatch)
+ throws IOException {
+ if (ifNoneMatch != null && "*".equals(ifNoneMatch.trim())) {
+ return getClientProtocol().createKeyIfNotExists(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ } else if (ifMatch != null) {
+ String expectedETag = parseETag(ifMatch);
+ return getClientProtocol().rewriteKeyIfMatch(
+ volumeName, bucketName, keyPath, length, expectedETag,
+ replicationConfig, customMetadata, tags);
+ } else {
+ return getClientProtocol().createKey(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ }
+ }
+
+ /**
+ * Parses an ETag from a conditional header value, removing surrounding
+ * quotes if present.
+ */
+ static String parseETag(String headerValue) {
+ if (headerValue == null) {
+ return null;
+ }
+ String etag = headerValue.trim();
+ if (etag.startsWith("\"") && etag.endsWith("\"")) {
+ return etag.substring(1, etag.length() - 1);
+ }
+ return etag;
Review Comment:
parseETag duplicates quote-stripping logic that already exists as S3Utils.stripQuotes (and is statically imported in this class). Consider reusing stripQuotes(headerValue.trim()) to keep ETag normalization consistent across the endpoint and reduce duplicate parsing behavior.
```suggestion
return stripQuotes(headerValue.trim());
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,48 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName,
+ OzoneBucket bucket, String keyPath, long length,
+ ReplicationConfig replicationConfig, Map<String, String> customMetadata,
+ Map<String, String> tags, String ifNoneMatch, String ifMatch)
+ throws IOException {
+ if (ifNoneMatch != null && "*".equals(ifNoneMatch.trim())) {
+ return getClientProtocol().createKeyIfNotExists(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ } else if (ifMatch != null) {
+ String expectedETag = parseETag(ifMatch);
+ return getClientProtocol().rewriteKeyIfMatch(
+ volumeName, bucketName, keyPath, length, expectedETag,
+ replicationConfig, customMetadata, tags);
+ } else {
Review Comment:
If-Match supports the standard `*` form in HTTP to mean "any current representation". Right now `If-Match: *` will be treated as an expected ETag of `*` and will always fail unless the stored ETag is literally `*`. Consider explicitly handling `*` (or returning INVALID_REQUEST if you intentionally don't support it) so behavior is not surprising for clients.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3019841629
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -281,21 +299,24 @@ Response handlePutRequest(ObjectRequestContext context, String keyPath, InputStr
getCustomMetadataFromHeaders(getHeaders().getRequestHeaders());
Map<String, String> tags = getTaggingFromHeaders(getHeaders());
+ boolean hasConditionalHeaders = ifNoneMatch != null || ifMatch != null;
long putLength;
final String md5Hash;
- if (isDatastreamEnabled() && !enableEC && length > getDatastreamMinLength()) {
+ if (isDatastreamEnabled() && !enableEC
+ && length > getDatastreamMinLength() && !hasConditionalHeaders) {
perf.appendStreamMode();
Pair<String, Long> keyWriteResult = ObjectEndpointStreaming
.put(bucket, keyPath, length, replicationConfig, getChunkSize(),
- customMetadata, tags, multiDigestInputStream, getHeaders(), signatureInfo.isSignPayload(), perf);
+ customMetadata, tags, multiDigestInputStream, getHeaders(),
+ signatureInfo.isSignPayload(), perf);
Review Comment:
PutObject using streaming write should be able to handle conditional request since the OpenKey and CommitKey operations are the same as the non-streaming put.
We can then remove the `hasConditionalHeaders` boolean.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -271,6 +280,15 @@ Response handlePutRequest(ObjectRequestContext context, String keyPath, InputStr
return Response.ok().status(HttpStatus.SC_OK).build();
}
+ String ifNoneMatch = getHeaders().getHeaderString(
+ S3Consts.IF_NONE_MATCH_HEADER);
+ String ifMatch = getHeaders().getHeaderString(
+ S3Consts.IF_MATCH_HEADER);
+
+ if (ifNoneMatch != null && ifMatch != null) {
+ throw newError(INVALID_REQUEST, keyPath);
+ }
Review Comment:
Can put an error message here since the `INVALID_REQUEST` is used in a lot of places.
Also we can do more validation
- Fail for blank header
- Fail if `If-None-Match` is not "*"
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,48 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName,
+ OzoneBucket bucket, String keyPath, long length,
+ ReplicationConfig replicationConfig, Map<String, String> customMetadata,
+ Map<String, String> tags, String ifNoneMatch, String ifMatch)
+ throws IOException {
+ if (ifNoneMatch != null && "*".equals(ifNoneMatch.trim())) {
+ return getClientProtocol().createKeyIfNotExists(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ } else if (ifMatch != null) {
+ String expectedETag = parseETag(ifMatch);
+ return getClientProtocol().rewriteKeyIfMatch(
+ volumeName, bucketName, keyPath, length, expectedETag,
+ replicationConfig, customMetadata, tags);
+ } else {
Review Comment:
This seems to be valid behavior based on the https://datatracker.ietf.org/doc/html/rfc7232#section-3.1. Should we implement this?
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,43 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName, String keyPath, long length,
+ ReplicationConfig replicationConfig, Map<String, String> customMetadata,
+ Map<String, String> tags, String ifNoneMatch, String ifMatch)
+ throws IOException {
+ if (ifNoneMatch != null && "*".equals(stripQuotes(ifNoneMatch.trim()))) {
+ return getClientProtocol().createKeyIfNotExists(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ } else if (ifMatch != null) {
+ String expectedETag = parseETag(ifMatch);
+ return getClientProtocol().rewriteKeyIfMatch(
+ volumeName, bucketName, keyPath, length, expectedETag,
+ replicationConfig, customMetadata, tags);
+ } else {
+ return getClientProtocol().createKey(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ }
+ }
+
+ /**
+ * Parses an ETag from a conditional header value, removing surrounding
+ * quotes if present.
+ */
+ static String parseETag(String headerValue) {
+ if (headerValue == null) {
+ return null;
+ }
+ return stripQuotes(headerValue.trim());
+ }
Review Comment:
Can move this to `S3Utils`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3019238708
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1435,6 +1435,68 @@ public OzoneOutputStream rewriteKey(String volumeName, String bucketName, String
return createOutputStream(openKey);
}
+ @Override
+ public OzoneOutputStream createKeyIfNotExists(String volumeName,
+ String bucketName, String keyName, long size,
+ ReplicationConfig replicationConfig, Map<String, String> metadata,
+ Map<String, String> tags) throws IOException {
+ if (omVersion.compareTo(OzoneManagerVersion.ATOMIC_REWRITE_KEY) < 0) {
+ throw new IOException(
+ "OzoneManager does not support atomic key creation.");
+ }
+
+ createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig);
+
+ OmKeyArgs.Builder builder = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(keyName)
+ .setDataSize(size)
+ .setReplicationConfig(replicationConfig)
+ .addAllMetadataGdpr(metadata)
+ .addAllTags(tags)
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setExpectedDataGeneration(
+ OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS);
+
+ OpenKeySession openKey = ozoneManagerClient.openKey(builder.build());
+ if (isS3GRequest.get() && size == 0) {
+ openKey.getKeyInfo().setDataSize(0);
+ }
+ return createOutputStream(openKey);
+ }
+
+ @Override
+ @SuppressWarnings("checkstyle:parameternumber")
+ public OzoneOutputStream rewriteKeyIfMatch(String volumeName,
Review Comment:
I refactor to let create/rewrite key type request to share same keyArgs builder, but I keep those RpcClient interface separate their responsibility eg. `rewriteKey()` for generation-based CAS and rewriteKeyIfMatch for ETAG-based CAS
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1435,6 +1435,68 @@ public OzoneOutputStream rewriteKey(String volumeName, String bucketName, String
return createOutputStream(openKey);
}
+ @Override
+ public OzoneOutputStream createKeyIfNotExists(String volumeName,
Review Comment:
https://github.com/apache/ozone/pull/9815#discussion_r3019238708
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3020567161
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -271,6 +280,38 @@ Response handlePutRequest(ObjectRequestContext context, String keyPath, InputStr
return Response.ok().status(HttpStatus.SC_OK).build();
}
+ String ifNoneMatch = getHeaders().getHeaderString(
Review Comment:
I’m still thinking about how to refactor this, as the conditional request validation logic dominates the function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3019841629
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -281,21 +299,24 @@ Response handlePutRequest(ObjectRequestContext context, String keyPath, InputStr
getCustomMetadataFromHeaders(getHeaders().getRequestHeaders());
Map<String, String> tags = getTaggingFromHeaders(getHeaders());
+ boolean hasConditionalHeaders = ifNoneMatch != null || ifMatch != null;
long putLength;
final String md5Hash;
- if (isDatastreamEnabled() && !enableEC && length > getDatastreamMinLength()) {
+ if (isDatastreamEnabled() && !enableEC
+ && length > getDatastreamMinLength() && !hasConditionalHeaders) {
perf.appendStreamMode();
Pair<String, Long> keyWriteResult = ObjectEndpointStreaming
.put(bucket, keyPath, length, replicationConfig, getChunkSize(),
- customMetadata, tags, multiDigestInputStream, getHeaders(), signatureInfo.isSignPayload(), perf);
+ customMetadata, tags, multiDigestInputStream, getHeaders(),
+ signatureInfo.isSignPayload(), perf);
Review Comment:
PutObject using streaming write should be able to handle conditional request since the OpenKey and CommitKey metadata operations are the same as the non-streaming put (only data operations are different).
We can then remove the `hasConditionalHeaders` boolean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #9815:
URL: https://github.com/apache/ozone/pull/9815#issuecomment-4144195448
Please rebase @peterxcli
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3020538030
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -281,21 +299,24 @@ Response handlePutRequest(ObjectRequestContext context, String keyPath, InputStr
getCustomMetadataFromHeaders(getHeaders().getRequestHeaders());
Map<String, String> tags = getTaggingFromHeaders(getHeaders());
+ boolean hasConditionalHeaders = ifNoneMatch != null || ifMatch != null;
long putLength;
final String md5Hash;
- if (isDatastreamEnabled() && !enableEC && length > getDatastreamMinLength()) {
+ if (isDatastreamEnabled() && !enableEC
+ && length > getDatastreamMinLength() && !hasConditionalHeaders) {
perf.appendStreamMode();
Pair<String, Long> keyWriteResult = ObjectEndpointStreaming
.put(bucket, keyPath, length, replicationConfig, getChunkSize(),
- customMetadata, tags, multiDigestInputStream, getHeaders(), signatureInfo.isSignPayload(), perf);
+ customMetadata, tags, multiDigestInputStream, getHeaders(),
+ signatureInfo.isSignPayload(), perf);
Review Comment:
ok, thanks for the context!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3022994769
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -271,6 +280,38 @@ Response handlePutRequest(ObjectRequestContext context, String keyPath, InputStr
return Response.ok().status(HttpStatus.SC_OK).build();
}
+ String ifNoneMatch = getHeaders().getHeaderString(
Review Comment:
https://issues.apache.org/jira/browse/HDDS-14958
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang merged PR #9815:
URL: https://github.com/apache/ozone/pull/9815
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3013645782
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1176,6 +1185,11 @@ message KeyInfo {
// This allows a key to be created an committed atomically if the original has not
// been modified.
optional uint64 expectedDataGeneration = 22;
+
+ // expectedETag, when set, indicates that the existing key must have
+ // the given ETag for the operation to succeed. This is used for
+ // S3 conditional writes with the If-Match header.
+ optional string expectedETag = 23;
Review Comment:
Hm, I don't recall that we need to add `expectedDataGeneration` and `expectedETag` in `KeyInfo`. Adding the two fields in KeyInfo requires additional logic to null the two fields during commit to prevent space overhead, but adding fields on two fields that will always be null in the keyTable does not seem to be right. However, I don't think we can avoid it since without setting the fields in the openKey, there might be concurrent PutObjects that might violate the serial consistency guarantee. Ideally, we should use a separate `OpenKeyInfo` for `openKeyTable` to differentiate with the final `KeyInfo` in the `keyTable`, but I guess the ship has sailed a long time ago.
Please let me know what you think. I'm OK if there are no other ways.
##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v1/AbstractS3SDKV1Tests.java:
##########
@@ -377,6 +377,103 @@ public void testPutObject() {
assertEquals("37b51d194a7513e45b56f6524f2d51f2", putObjectResult.getETag());
}
+ @Test
+ public void testPutObjectIfNoneMatch() {
+ final String bucketName = getBucketName();
+ final String keyName = getKeyName();
+ final String content = "bar";
+ s3Client.createBucket(bucketName);
+
+ InputStream is = new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
+ ObjectMetadata metadata = new ObjectMetadata();
+ metadata.setHeader("If-None-Match", "*");
+
+ PutObjectResult putObjectResult = s3Client.putObject(bucketName, keyName, is, metadata);
+ assertEquals("37b51d194a7513e45b56f6524f2d51f2", putObjectResult.getETag());
+ }
+
+ @Test
+ public void testPutObjectIfNoneMatchFail() {
+ final String bucketName = getBucketName();
+ final String keyName = getKeyName();
+ final String content = "bar";
+ s3Client.createBucket(bucketName);
+
+ InputStream is = new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
+ s3Client.putObject(bucketName, keyName, is, new ObjectMetadata());
+
+ InputStream is2 = new ByteArrayInputStream(content.getBytes(StandardCharsets.UTF_8));
+ ObjectMetadata metadata = new ObjectMetadata();
+ metadata.setHeader("If-None-Match", "*");
+
+ AmazonServiceException ase = assertThrows(AmazonServiceException.class,
+ () -> s3Client.putObject(bucketName, keyName, is2, metadata));
+
+ assertEquals(ErrorType.Client, ase.getErrorType());
+ assertEquals(412, ase.getStatusCode());
+ assertEquals("PreconditionFailed", ase.getErrorCode());
+ }
Review Comment:
Nit: Please also help to add the post validation suggested in the acceptance tests for relevant integration test methods (V1 and V2). Thanks.
##########
hadoop-ozone/dist/src/main/smoketest/s3/conditionalput.robot:
##########
@@ -0,0 +1,77 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+*** Settings ***
+Documentation S3 Conditional Put (If-None-Match / If-Match) tests
+Library OperatingSystem
+Library String
+Library Process
+Resource ../commonlib.robot
+Resource ./commonawslib.robot
+Test Timeout 5 minutes
+Suite Setup Setup s3 tests
+
+*** Variables ***
+${ENDPOINT_URL} http://s3g:9878
+${BUCKET} generated
+
+*** Test Cases ***
+
+Conditional Put If-None-Match Star Creates New Key
+ [Documentation] If-None-Match: * should succeed when key does not exist
+ ${key} = Set Variable condput-ifnonematch-new
+ Execute echo "test-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} ETag
+
+Conditional Put If-None-Match Star Fails For Existing Key
+ [Documentation] If-None-Match: * should fail with 412 when key already exists
+ ${key} = Set Variable condput-ifnonematch-existing
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Now try again with If-None-Match: *
+ ${result} = Execute AWSS3APICli and ignore error put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} PreconditionFailed
+
+Conditional Put If-Match With Correct ETag Succeeds
+ [Documentation] If-Match with correct ETag should succeed
+ ${key} = Set Variable condput-ifmatch-success
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Extract the ETag value
+ ${etag} = Evaluate __import__('json').loads(r'''${result}''')['ETag'].strip('"')
+ # Rewrite with matching ETag
+ Execute echo "updated-content" > /tmp/${key}-updated
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}-updated --if-match ${etag}
+ Should contain ${result} ETag
Review Comment:
Nit: Can compare the `result` with the previous `etag`.
##########
hadoop-ozone/dist/src/main/smoketest/s3/conditionalput.robot:
##########
@@ -0,0 +1,77 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+*** Settings ***
+Documentation S3 Conditional Put (If-None-Match / If-Match) tests
+Library OperatingSystem
+Library String
+Library Process
+Resource ../commonlib.robot
+Resource ./commonawslib.robot
+Test Timeout 5 minutes
+Suite Setup Setup s3 tests
+
+*** Variables ***
+${ENDPOINT_URL} http://s3g:9878
+${BUCKET} generated
+
+*** Test Cases ***
+
+Conditional Put If-None-Match Star Creates New Key
+ [Documentation] If-None-Match: * should succeed when key does not exist
+ ${key} = Set Variable condput-ifnonematch-new
+ Execute echo "test-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} ETag
+
+Conditional Put If-None-Match Star Fails For Existing Key
+ [Documentation] If-None-Match: * should fail with 412 when key already exists
+ ${key} = Set Variable condput-ifnonematch-existing
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Now try again with If-None-Match: *
+ ${result} = Execute AWSS3APICli and ignore error put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} PreconditionFailed
+
+Conditional Put If-Match With Correct ETag Succeeds
+ [Documentation] If-Match with correct ETag should succeed
+ ${key} = Set Variable condput-ifmatch-success
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Extract the ETag value
+ ${etag} = Evaluate __import__('json').loads(r'''${result}''')['ETag'].strip('"')
+ # Rewrite with matching ETag
+ Execute echo "updated-content" > /tmp/${key}-updated
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}-updated --if-match ${etag}
+ Should contain ${result} ETag
+
+Conditional Put If-Match With Wrong ETag Fails
+ [Documentation] If-Match with wrong ETag should fail with 412
+ ${key} = Set Variable condput-ifmatch-fail
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Try to rewrite with a wrong ETag
+ ${result} = Execute AWSS3APICli and ignore error put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-match wrong-etag
+ Should contain ${result} PreconditionFailed
Review Comment:
Nit: We can query the existing original key and ensure it's indeed not rewritten (checking the ETag using HeadObject should be enough).
##########
hadoop-ozone/dist/src/main/smoketest/s3/conditionalput.robot:
##########
@@ -0,0 +1,77 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+*** Settings ***
+Documentation S3 Conditional Put (If-None-Match / If-Match) tests
+Library OperatingSystem
+Library String
+Library Process
+Resource ../commonlib.robot
+Resource ./commonawslib.robot
+Test Timeout 5 minutes
+Suite Setup Setup s3 tests
+
+*** Variables ***
+${ENDPOINT_URL} http://s3g:9878
+${BUCKET} generated
+
+*** Test Cases ***
+
+Conditional Put If-None-Match Star Creates New Key
+ [Documentation] If-None-Match: * should succeed when key does not exist
+ ${key} = Set Variable condput-ifnonematch-new
+ Execute echo "test-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} ETag
+
+Conditional Put If-None-Match Star Fails For Existing Key
+ [Documentation] If-None-Match: * should fail with 412 when key already exists
+ ${key} = Set Variable condput-ifnonematch-existing
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Now try again with If-None-Match: *
+ ${result} = Execute AWSS3APICli and ignore error put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-none-match "*"
+ Should contain ${result} PreconditionFailed
+
+Conditional Put If-Match With Correct ETag Succeeds
+ [Documentation] If-Match with correct ETag should succeed
+ ${key} = Set Variable condput-ifmatch-success
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Extract the ETag value
+ ${etag} = Evaluate __import__('json').loads(r'''${result}''')['ETag'].strip('"')
+ # Rewrite with matching ETag
+ Execute echo "updated-content" > /tmp/${key}-updated
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}-updated --if-match ${etag}
+ Should contain ${result} ETag
+
+Conditional Put If-Match With Wrong ETag Fails
+ [Documentation] If-Match with wrong ETag should fail with 412
+ ${key} = Set Variable condput-ifmatch-fail
+ Execute echo "initial-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key}
+ Should contain ${result} ETag
+ # Try to rewrite with a wrong ETag
+ ${result} = Execute AWSS3APICli and ignore error put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-match wrong-etag
+ Should contain ${result} PreconditionFailed
+
+Conditional Put If-Match On Non-Existent Key Fails
+ [Documentation] If-Match on a key that does not exist should fail with 412
+ ${key} = Set Variable condput-ifmatch-nonexistent
+ Execute echo "test-content" > /tmp/${key}
+ ${result} = Execute AWSS3APICli and ignore error put-object --bucket ${BUCKET} --key ${key} --body /tmp/${key} --if-match some-etag
+ Should contain ${result} PreconditionFailed
Review Comment:
Nit: Check that the key does not exist.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #9815:
URL: https://github.com/apache/ozone/pull/9815#issuecomment-4168992944
@ivandika3 please take another look, Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3020529584
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -281,21 +299,24 @@ Response handlePutRequest(ObjectRequestContext context, String keyPath, InputStr
getCustomMetadataFromHeaders(getHeaders().getRequestHeaders());
Map<String, String> tags = getTaggingFromHeaders(getHeaders());
+ boolean hasConditionalHeaders = ifNoneMatch != null || ifMatch != null;
long putLength;
final String md5Hash;
- if (isDatastreamEnabled() && !enableEC && length > getDatastreamMinLength()) {
+ if (isDatastreamEnabled() && !enableEC
+ && length > getDatastreamMinLength() && !hasConditionalHeaders) {
perf.appendStreamMode();
Pair<String, Long> keyWriteResult = ObjectEndpointStreaming
.put(bucket, keyPath, length, replicationConfig, getChunkSize(),
- customMetadata, tags, multiDigestInputStream, getHeaders(), signatureInfo.isSignPayload(), perf);
+ customMetadata, tags, multiDigestInputStream, getHeaders(),
+ signatureInfo.isSignPayload(), perf);
Review Comment:
@peterxcli Seems you need to introduce conditional APIs for stream APIs (i.e. `createStreamKey`) and pass the related conditional request parameter (ifNoneMatch, etc) to `ObjectEndpointStreaming` or reparse them.
We also need to find out why the SDK integration test `TestS3SDKWithRatisStreaming` does not fail the conditional request when they are not supported.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3022976423
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1176,6 +1185,11 @@ message KeyInfo {
// This allows a key to be created an committed atomically if the original has not
// been modified.
optional uint64 expectedDataGeneration = 22;
+
+ // expectedETag, when set, indicates that the existing key must have
+ // the given ETag for the operation to succeed. This is used for
+ // S3 conditional writes with the If-Match header.
+ optional string expectedETag = 23;
Review Comment:
Raised a ticket for this: https://issues.apache.org/jira/browse/HDDS-14957
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #9815:
URL: https://github.com/apache/ozone/pull/9815#issuecomment-4170993997
Thanks @ivandika3, @jojochuang for reviewing!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3019861271
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,43 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName, String keyPath, long length,
+ ReplicationConfig replicationConfig, Map<String, String> customMetadata,
+ Map<String, String> tags, String ifNoneMatch, String ifMatch)
+ throws IOException {
+ if (ifNoneMatch != null && "*".equals(stripQuotes(ifNoneMatch.trim()))) {
+ return getClientProtocol().createKeyIfNotExists(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ } else if (ifMatch != null) {
+ String expectedETag = parseETag(ifMatch);
+ return getClientProtocol().rewriteKeyIfMatch(
+ volumeName, bucketName, keyPath, length, expectedETag,
+ replicationConfig, customMetadata, tags);
+ } else {
+ return getClientProtocol().createKey(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ }
+ }
+
+ /**
+ * Parses an ETag from a conditional header value, removing surrounding
+ * quotes if present.
+ */
+ static String parseETag(String headerValue) {
+ if (headerValue == null) {
+ return null;
+ }
+ return stripQuotes(headerValue.trim());
+ }
Review Comment:
Can move this to `S3Utils`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3019990939
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1127,7 +1148,48 @@ private CopyObjectResponse copyObject(OzoneVolume volume,
}
}
}
-
+
+ /**
+ * Opens a key for put, applying conditional write logic based on
+ * If-None-Match and If-Match headers.
+ */
+ @SuppressWarnings("checkstyle:ParameterNumber")
+ private OzoneOutputStream openKeyForPut(String volumeName, String bucketName,
+ OzoneBucket bucket, String keyPath, long length,
+ ReplicationConfig replicationConfig, Map<String, String> customMetadata,
+ Map<String, String> tags, String ifNoneMatch, String ifMatch)
+ throws IOException {
+ if (ifNoneMatch != null && "*".equals(ifNoneMatch.trim())) {
+ return getClientProtocol().createKeyIfNotExists(
+ volumeName, bucketName, keyPath, length, replicationConfig,
+ customMetadata, tags);
+ } else if (ifMatch != null) {
+ String expectedETag = parseETag(ifMatch);
+ return getClientProtocol().rewriteKeyIfMatch(
+ volumeName, bucketName, keyPath, length, expectedETag,
+ replicationConfig, customMetadata, tags);
+ } else {
Review Comment:
you're right.
https://github.com/minio/minio/blob/7aac2a2c5b7c882e68c1ce017d8256be2feea27f/cmd/object-handlers-common.go#L343-L350
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r2898576271
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/exceptions/OMException.java:
##########
@@ -267,13 +267,17 @@ public enum ResultCodes {
UNAUTHORIZED,
S3_SECRET_ALREADY_EXISTS,
-
+
Review Comment:
please do not commit changes that unrelated.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1428,6 +1428,68 @@ public OzoneOutputStream rewriteKey(String volumeName, String bucketName, String
return createOutputStream(openKey);
}
+ @Override
+ public OzoneOutputStream createKeyIfNotExists(String volumeName,
+ String bucketName, String keyName, long size,
+ ReplicationConfig replicationConfig, Map<String, String> metadata,
+ Map<String, String> tags) throws IOException {
+ if (omVersion.compareTo(OzoneManagerVersion.ATOMIC_REWRITE_KEY) < 0) {
+ throw new IOException(
+ "OzoneManager does not support atomic key creation.");
+ }
+
+ createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig);
+
+ OmKeyArgs.Builder builder = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(keyName)
+ .setDataSize(size)
+ .setReplicationConfig(replicationConfig)
+ .addAllMetadataGdpr(metadata)
+ .addAllTags(tags)
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setExpectedDataGeneration(
+ OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS);
+
+ OpenKeySession openKey = ozoneManagerClient.openKey(builder.build());
+ if (isS3GRequest.get() && size == 0) {
+ openKey.getKeyInfo().setDataSize(0);
+ }
+ return createOutputStream(openKey);
+ }
+
+ @Override
+ @SuppressWarnings("checkstyle:parameternumber")
+ public OzoneOutputStream rewriteKeyIfMatch(String volumeName,
+ String bucketName, String keyName, long size, String expectedETag,
+ ReplicationConfig replicationConfig, Map<String, String> metadata,
+ Map<String, String> tags) throws IOException {
+ if (omVersion.compareTo(OzoneManagerVersion.ATOMIC_REWRITE_KEY) < 0) {
+ throw new IOException(
+ "OzoneManager does not support conditional key rewrite.");
+ }
+
+ createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig);
+
+ OmKeyArgs.Builder builder = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(keyName)
+ .setDataSize(size)
+ .setReplicationConfig(replicationConfig)
+ .addAllMetadataGdpr(metadata)
+ .addAllTags(tags)
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setExpectedETag(expectedETag);
Review Comment:
does it set setExpectedDataGeneration here?
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -187,8 +188,18 @@ public Response put(
throw newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName, ex);
} else if (ex.getResult() == ResultCodes.FILE_ALREADY_EXISTS) {
throw newError(S3ErrorTable.NO_OVERWRITE, keyPath, ex);
+ } else if (ex.getResult() == ResultCodes.KEY_ALREADY_EXISTS) {
+ throw newError(PRECOND_FAILED, keyPath, ex);
Review Comment:
would you like to consider having different error messages for different cases?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r3020529584
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -281,21 +299,24 @@ Response handlePutRequest(ObjectRequestContext context, String keyPath, InputStr
getCustomMetadataFromHeaders(getHeaders().getRequestHeaders());
Map<String, String> tags = getTaggingFromHeaders(getHeaders());
+ boolean hasConditionalHeaders = ifNoneMatch != null || ifMatch != null;
long putLength;
final String md5Hash;
- if (isDatastreamEnabled() && !enableEC && length > getDatastreamMinLength()) {
+ if (isDatastreamEnabled() && !enableEC
+ && length > getDatastreamMinLength() && !hasConditionalHeaders) {
perf.appendStreamMode();
Pair<String, Long> keyWriteResult = ObjectEndpointStreaming
.put(bucket, keyPath, length, replicationConfig, getChunkSize(),
- customMetadata, tags, multiDigestInputStream, getHeaders(), signatureInfo.isSignPayload(), perf);
+ customMetadata, tags, multiDigestInputStream, getHeaders(),
+ signatureInfo.isSignPayload(), perf);
Review Comment:
@peterxcli Seems you need to introduce conditional APIs for stream APIs (i.e. `createStreamKey`) and pass the related conditional request parameter (ifNoneMatch, etc) to `ObjectEndpointStreaming` or reparse the header in `ObjectEndpointStreaming` itself.
We also need to find out why the SDK integration test `TestS3SDKWithRatisStreaming` does not fail the conditional request when they are not supported.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-13919. S3 Conditional Writes (PutObject) [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #9815:
URL: https://github.com/apache/ozone/pull/9815#discussion_r2986933629
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -1428,6 +1428,68 @@ public OzoneOutputStream rewriteKey(String volumeName, String bucketName, String
return createOutputStream(openKey);
}
+ @Override
+ public OzoneOutputStream createKeyIfNotExists(String volumeName,
+ String bucketName, String keyName, long size,
+ ReplicationConfig replicationConfig, Map<String, String> metadata,
+ Map<String, String> tags) throws IOException {
+ if (omVersion.compareTo(OzoneManagerVersion.ATOMIC_REWRITE_KEY) < 0) {
+ throw new IOException(
+ "OzoneManager does not support atomic key creation.");
+ }
+
+ createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig);
+
+ OmKeyArgs.Builder builder = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(keyName)
+ .setDataSize(size)
+ .setReplicationConfig(replicationConfig)
+ .addAllMetadataGdpr(metadata)
+ .addAllTags(tags)
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setExpectedDataGeneration(
+ OzoneConsts.EXPECTED_GEN_CREATE_IF_NOT_EXISTS);
+
+ OpenKeySession openKey = ozoneManagerClient.openKey(builder.build());
+ if (isS3GRequest.get() && size == 0) {
+ openKey.getKeyInfo().setDataSize(0);
+ }
+ return createOutputStream(openKey);
+ }
+
+ @Override
+ @SuppressWarnings("checkstyle:parameternumber")
+ public OzoneOutputStream rewriteKeyIfMatch(String volumeName,
+ String bucketName, String keyName, long size, String expectedETag,
+ ReplicationConfig replicationConfig, Map<String, String> metadata,
+ Map<String, String> tags) throws IOException {
+ if (omVersion.compareTo(OzoneManagerVersion.ATOMIC_REWRITE_KEY) < 0) {
+ throw new IOException(
+ "OzoneManager does not support conditional key rewrite.");
+ }
+
+ createKeyPreChecks(volumeName, bucketName, keyName, replicationConfig);
+
+ OmKeyArgs.Builder builder = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(keyName)
+ .setDataSize(size)
+ .setReplicationConfig(replicationConfig)
+ .addAllMetadataGdpr(metadata)
+ .addAllTags(tags)
+ .setLatestVersionLocation(getLatestVersionLocation)
+ .setExpectedETag(expectedETag);
Review Comment:
no, please take a look at the design doc:
https://github.com/apache/ozone/blob/5e5243eca2a28ba5127be5e10bba97a99adf9d52/hadoop-hdds/docs/content/design/s3-conditional-requests.md?plain=1#L144-L167
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org