You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/09/13 17:25:48 UTC

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

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



##########
File path: hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestMultipartUploadWithCopy.java
##########
@@ -172,103 +173,132 @@ public void testMultipart() throws Exception {
     }
   }
 
-  /* The next two tests exercise all the combinations of modification times.
-   * There are two types times, ModifiedSince, and UnmodifiedSince.  Each of
-   * those can be in one of 5 states:
-   * 1. Valid and True
-   * 2. Valid and False
-   * 3. Null
-   * 4. Invalid/Unparseable
-   * 5. Invalid/Future time
-   * Which means there are 25, (5*5), combinations of the two, all of which
-   * are tried below.
-   * The comments above each test list the states for each type being
-   * tested
-   */
-  @Test
-  public void testMultipartIfModifiedIsFalse() throws Exception {
-    // False/ifModifiedSince = afterSourceKeyModificationTime
-    // True/ifUnmodifiedSince = afterSourceKeyModificationTime
-    try {
-      uploadPartWithCopy(
-          afterSourceKeyModificationTimeStr,
-          afterSourceKeyModificationTimeStr
-      );
-      fail("testMultipartIfModifiedIsFalse");
-    } catch (OS3Exception ex) {
-      assertEquals(ex.getCode(), S3ErrorTable.PRECOND_FAILED.getCode());
-    }
-
-    // False/ifModifiedSince = afterSourceKeyModificationTime
-    // False/ifUnmodifiedSince = beforeSourceKeyModificationTime,
-    try {
-      uploadPartWithCopy(
-          afterSourceKeyModificationTimeStr,
-          beforeSourceKeyModificationTimeStr
-      );
-      fail("testMultipartIfModifiedIsFalse");
-    } catch (OS3Exception ex) {
-      assertEquals(ex.getCode(), S3ErrorTable.PRECOND_FAILED.getCode());
+  /**
+  * CopyIfTimestampTestCase captures all the possibilities for the time stamps
+  * that can be passed into the multipart copy with copy-if flags for
+  * timestamps. Only some of the cases are valid others should raise an
+  * exception.
+  * Time stamps can be,
+  * 1. after the timestamp on the object but still a valid time stamp
+  * (in regard to wall clock time on server)
+  * 2. before the timestamp on the object
+  * 3. In the Future beyond the wall clock time on the server
+  * 4. Null
+  * 5. Unparsable
+  */
+  public enum CopyIfTimestampTestCase {
+    MODIFIED_SINCE_AFTER_TS_UNMODIFIED_SINCE_AFTER_TS(
+        afterSourceKeyModificationTimeStr, afterSourceKeyModificationTimeStr,
+        ERROR_CODE),
+    MODIFIED_SINCE_AFTER_TS_UNMODIFIED_SINCE_BEFORE_TS(
+        afterSourceKeyModificationTimeStr, beforeSourceKeyModificationTimeStr,
+        ERROR_CODE),
+    MODIFIED_SINCE_AFTER_TS_UNMODIFIED_SINCE_NULL(
+        afterSourceKeyModificationTimeStr, null,
+        ERROR_CODE),
+    MODIFIED_SINCE_AFTER_TS_UNMODIFIED_SINCE_FUTURE(
+        afterSourceKeyModificationTimeStr, futureTimeStr,
+        ERROR_CODE),
+    MODIFIED_SINCE_AFTER_TS_UNMODIFIED_SINCE_UNPARSABLE_TS(
+        afterSourceKeyModificationTimeStr, UNPARSABLE_TIME_STR,
+        ERROR_CODE),
+
+    MODIFIED_SINCE_BEFORE_TS_UNMODIFIED_SINCE_AFTER_TS(
+        beforeSourceKeyModificationTimeStr, afterSourceKeyModificationTimeStr,
+        null),
+    MODIFIED_SINCE_BEFORE_TS_UNMODIFIED_SINCE_BEFORE_TS(
+        beforeSourceKeyModificationTimeStr, beforeSourceKeyModificationTimeStr,
+        ERROR_CODE),
+    MODIFIED_SINCE_BEFORE_TS_UNMODIFIED_SINCE_NULL(
+        beforeSourceKeyModificationTimeStr, null,
+        null),
+    MODIFIED_SINCE_BEFORE_TS_UNMOFIFIED_SINCE_FUTURE(
+        beforeSourceKeyModificationTimeStr, futureTimeStr,
+        null),
+    MODIFIED_SINCE_BEFORE_TS_UNMODIFIED_SINCE_UNPARSABLE_TS(
+        beforeSourceKeyModificationTimeStr, UNPARSABLE_TIME_STR,
+        null),
+
+    MODIFIED_SINCE_NULL_TS_UNMODIFIED_SINCE_AFTER_TS(
+        null, afterSourceKeyModificationTimeStr,
+        null),
+    MODIFIED_SINCE_NULL_TS_UNMODIFIED_SINCE_BEFORE_TS(
+        null, beforeSourceKeyModificationTimeStr,
+        ERROR_CODE),
+    MODIFIED_SINCE_NULL_TS_UNMODIFIED_SINCE_NULL_TS(
+        null, null,
+        null),
+    MODIFIED_SINCE_NULL_TS_UNMODIFIED_SINCE_FUTURE_TS(
+        null, futureTimeStr,
+        null),
+    MODIFIED_SINCE_NULL_TS_UNMODIFIED_SINCE_UNPARSABLE_TS(
+        null, UNPARSABLE_TIME_STR,
+        null),
+
+    MODIFIED_SINCE_UNPARSABLE_TS_UNMODIFIED_SINCE_AFTER_TS(
+        UNPARSABLE_TIME_STR, afterSourceKeyModificationTimeStr,
+        null),
+    MODIFIED_SINCE_UNPARSABLE_TS_UNMODIFIED_SINCE_BEFORE_TS(
+        UNPARSABLE_TIME_STR, beforeSourceKeyModificationTimeStr,
+        ERROR_CODE),
+    MODIFIED_SINCE_UNPARSABLE_TS_UNMODIFIED_SINCE_NULL_TS(
+        UNPARSABLE_TIME_STR, null,
+        null),
+    MODIFIED_SINCE_UNPARSABLE_TS_UNMODIFIED_SINCE_FUTURE_TS(
+        UNPARSABLE_TIME_STR, futureTimeStr,
+        null),
+    MODIFIED_SINCE_UNPARSABLE_TS_UNMODIFIED_SINCE_UNPARSABLE_TS(
+        UNPARSABLE_TIME_STR, UNPARSABLE_TIME_STR,
+        null),
+
+    MODIFIED_SINCE_FUTURE_TS_UNMODIFIED_SINCE_AFTER_TS(
+        futureTimeStr, afterSourceKeyModificationTimeStr,
+        null),
+    MODIFIED_SINCE_FUTURE_TS_UNMODIFIED_SINCE_BEFORE_TS(
+        futureTimeStr, beforeSourceKeyModificationTimeStr,
+        ERROR_CODE),
+    MODIFIED_SINCE_FUTURE_TS_UNMODIFIED_SINCE_NULL_TS(
+        futureTimeStr, null,
+        null),
+    MODIFIED_SINCE_FUTURE_TS_UNMODIFIED_SINCE_FUTURE_TS(
+        futureTimeStr, futureTimeStr,
+        null),
+    MODIFIED_SINCE_FUTURE_TS_UNMODIFIED_SINCE_UNPARSABLE_TS(
+        futureTimeStr, UNPARSABLE_TIME_STR,
+        null);
+    
+    private final String modifiedTimestamp;
+    private final String unmodifiedTimestamp;
+    private final String errorCode;
+
+    CopyIfTimestampTestCase(String modifiedTimestamp,
+        String unmodifiedTimestamp, String errorCode) {
+      this.modifiedTimestamp = modifiedTimestamp;
+      this.unmodifiedTimestamp = unmodifiedTimestamp;
+      this.errorCode = errorCode;
     }
 
-    // False/ifModifiedSince = afterSourceKeyModificationTime
-    // Null
-    try {
-      uploadPartWithCopy(afterSourceKeyModificationTimeStr, null);
-      fail("testMultipartIfModifiedIsFalse");
-    } catch (OS3Exception ex) {
-      assertEquals(ex.getCode(), S3ErrorTable.PRECOND_FAILED.getCode());
+    @Override
+    public String toString() {
+      return this.name() +
+          " Modified:" + this.modifiedTimestamp
+          + " Unmodified:" + this.unmodifiedTimestamp
+          + " ErrorCode:" + this.errorCode;
     }
-    // False/ifModifiedSince = afterSourceKeyModificationTime
-    // Future
-    try {
-      uploadPartWithCopy(afterSourceKeyModificationTimeStr, futureTimeStr);
-      fail("testMultipartIfModifiedIsFalse");
-    } catch (OS3Exception ex) {
-      assertEquals(ex.getCode(), S3ErrorTable.PRECOND_FAILED.getCode());
-    }
-    // False/ifModifiedSince = afterSourceKeyModificationTime
-    // Unparsable
-    try {
-      uploadPartWithCopy(afterSourceKeyModificationTimeStr,
-          UNPARSABLE_TIME_STR);
-      fail("testMultipartIfModifiedIsFalse");
-    } catch (OS3Exception ex) {
-      assertEquals(ex.getCode(), S3ErrorTable.PRECOND_FAILED.getCode());
-    }
-
   }
-  
   @Test
-  public void testMultipartIfModifiedIsTrueOrInvalid() throws Exception {
-    String[] trueOrInvalidTimes = {beforeSourceKeyModificationTimeStr,
-                                   null, UNPARSABLE_TIME_STR, futureTimeStr};
-
-    for (String ts: trueOrInvalidTimes) {
-      // True/Null/Unparsable/Future
-      // True/ifUnmodifiedSince = afterSourceKeyModificationTimeStr
-      uploadPartWithCopy(ts, afterSourceKeyModificationTimeStr);
-  
-      // True/Null/Unparsable/Future
-      // False/ifUnmodifiedSince = beforeSourceKeyModificationTime
+  public void testMultipartTSHeaders() throws Exception {
+    for (CopyIfTimestampTestCase t : CopyIfTimestampTestCase.values()) {
       try {
-        uploadPartWithCopy(ts, beforeSourceKeyModificationTimeStr);
-        fail("testMultipartIfModifiedIsTrueOrInvalid");
+        uploadPartWithCopy(t.modifiedTimestamp, t.unmodifiedTimestamp);
+        if (t.errorCode != null) {
+          fail("Fail test:" + t);
+        }
       } catch (OS3Exception ex) {
-        assertEquals(ex.getCode(), S3ErrorTable.PRECOND_FAILED.getCode());
+        if (t.errorCode == null) {

Review comment:
       yes, thanks @kerneltime!.  fixed now.




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