[GitHub] [ozone] adoroszlai commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests

[GitBox <gi...@apache.org>]

adoroszlai commented on a change in pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#discussion_r717276119



##########
File path: hadoop-ozone/dist/src/main/smoketest/s3/MultipartUpload.robot
##########
@@ -29,6 +29,9 @@ Create Random file
     [arguments]             ${size_in_megabytes}
     Execute                 dd if=/dev/urandom of=/tmp/part1 bs=1048576 count=${size_in_megabytes}
 
+Wait For AfterCreate
+    [arguments]             ${sleepTime}
+    Sleep                   ${sleepTime}

Review comment:
       What is the goal with this keyword?  Functionally it seems to be the same as `Sleep`.
   
   I would suggest extracting a few more lines from the test into a keyword instead, something like this:
   
   ```
   Wait Until Date Past
       [arguments]         ${date}
       ${latestDate} =     Get Current Date         UTC
       ${sleepSeconds} =   Subtract Date From Date  ${date}  ${latestDate}
       Run Keyword If      ${sleepSeconds} > 0      Sleep  ${sleepSeconds}
   ```

##########
File path: hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestMultipartUploadWithCopy.java
##########
@@ -89,6 +95,26 @@ public static void setUp() throws Exception {
       stream.write(keyContent);
     }
 
+    sourceKeyLastModificationTime = CLIENT.getObjectStore()
+        .getS3Bucket(OzoneConsts.S3_BUCKET)
+        .getKey(EXISTING_KEY)
+        .getModificationTime().toEpochMilli();
+    beforeSourceKeyModificationTimeStr =
+        OzoneUtils.formatTime(sourceKeyLastModificationTime - 1000);
+    afterSourceKeyModificationTimeStr =
+        OzoneUtils.formatTime(sourceKeyLastModificationTime + DELAY_MS);
+    futureTimeStr =
+        OzoneUtils.formatTime(sourceKeyLastModificationTime +
+            1000 * 60 * 24);
+
+    // Make sure DELAY_MS has passed, otherwise
+    //  afterSourceKeyModificationTimeStr will be in the future
+    //  and thus invalid
+    long currentTime = new Date().getTime();

Review comment:
       Same here:
   
   ```suggestion
       long currentTime = System.currentTimeMillis();
   ```

##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java
##########
@@ -858,15 +860,29 @@ private static int parsePartNumberMarker(String partNumberMarker) {
     return partMarker;
   }
 
-  private static long parseOzoneDate(String ozoneDateStr) throws OS3Exception {
+  // Parses date string and return long representation. Returns an
+  // empty if DateStr is null or invalid. Dates in the future are
+  // considered invalid.
+  private static OptionalLong parseAndValidateDate(String ozoneDateStr)
+      throws OS3Exception {

Review comment:
       No longer thrown:
   
   ```suggestion
     private static OptionalLong parseAndValidateDate(String ozoneDateStr) {
   ```
   
   (Also in `checkCopySourceModificationTime`.)

##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java
##########
@@ -858,15 +860,29 @@ private static int parsePartNumberMarker(String partNumberMarker) {
     return partMarker;
   }
 
-  private static long parseOzoneDate(String ozoneDateStr) throws OS3Exception {
+  // Parses date string and return long representation. Returns an
+  // empty if DateStr is null or invalid. Dates in the future are
+  // considered invalid.
+  private static OptionalLong parseAndValidateDate(String ozoneDateStr)
+      throws OS3Exception {
     long ozoneDateInMs;
+    if (ozoneDateStr == null) {
+      return OptionalLong.empty();
+    }
     try {
       ozoneDateInMs = OzoneUtils.formatDate(ozoneDateStr);
     } catch (ParseException e) {
-      throw S3ErrorTable.newError(S3ErrorTable
-          .INVALID_ARGUMENT, ozoneDateStr);
+      // if time not parseable, then return empty()
+      return OptionalLong.empty();
+    }
+
+    long currentDate = new Date().getTime();

Review comment:
       I think we can avoid creating a new `Date` object:
   
   ```suggestion
       long currentDate = System.currentTimeMillis();
   ```




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