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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
