ArafatKhan2198 commented on code in PR #10786:
URL: https://github.com/apache/ozone/pull/10786#discussion_r3620479039


##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/util/S3Utils.java:
##########
@@ -73,6 +74,22 @@ public static String s3urlEncode(String str)
     return urlEncode(str).replace("+", "%20");
   }
 
+  /**
+   * Returns the persisted S3 {@code Content-Encoding} value, with {@code 
aws-chunked}

Review Comment:
   Handle `aws-chunked` without case sensitivity
   
   Currently, only this exact value is removed:
   
   ```
   aws-chunked
   ```
   
   These would not be removed:
   
   ```
   AWS-CHUNKED
   Aws-Chunked
   ```
   
   HTTP encoding names are case-insensitive, so the code should use:
   
   ```
   equalsIgnoreCase()
   ```



##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java:
##########
@@ -120,15 +120,26 @@ public abstract class EndpointBase {
 
   protected static final String ETAG_CUSTOM = "etag-custom";
   protected static final String CONTENT_TYPE_CUSTOM = "content-type-custom";
+  protected static final String CACHE_CONTROL_CUSTOM = "cache-control-custom";

Review Comment:
   The PR internally renames:
   
   ```
   x-amz-meta-cache-control
   ```
   
   to:
   
   ```
   cache-control-custom
   ```
   
   But a user can also provide:
   
   ```
   x-amz-meta-cache-control-custom
   ```
   
   Both values then use the same internal key, so one value can overwrite the 
other.
   The internal storage key should use a reserved namespace that users cannot 
create.



##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestObjectGet.java:
##########
@@ -221,12 +221,12 @@ public void getKeyWithTag() throws IOException, 
OS3Exception {
   }
 
   @Test
-  public void inheritRequestHeader() throws IOException, OS3Exception {
+  public void storedObjectHeadersOnGetAndHead() throws IOException, 
OS3Exception {

Review Comment:
   Add tests for copy and multipart upload
   
   The code changes normal upload, multipart upload, and object-copy behaviour.
   
   However, the integration tests mainly verify normal `PutObject`.
   
   Tests should also confirm that headers work correctly when:
   
   - uploading through multipart upload;
   - copying metadata with `COPY`;
   - replacing metadata with `REPLACE`.
   
   Overall, the main feature is implemented correctly, but these edge cases 
could cause incorrect headers or lost metadata.



-- 
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]

Reply via email to