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/08/06 18:13:19 UTC
[GitHub] [ozone] GeorgeJahad opened a new pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
GeorgeJahad opened a new pull request #2509:
URL: https://github.com/apache/ozone/pull/2509
## What changes were proposed in this pull request?
When running the [s3 compatibility tests](https://github.com/apache/ozone/tree/master/hadoop-ozone/dist/src/main/smoketest/s3/s3_compatbility_check.sh), I noticed AWS ignores invalid date strings in the copy-source-parameters, (copySourceIfModifiedSince, and copySourceIfUnmodifiedSince) of the upload-part-copy cmd.
Date strings in the future are also considered invalid.
I was unable to find anything that documented this behaviour explicitly. I'm just going on the results from the compatibility test.
@kerneltime mentioned that rfc2616 describes the behaviour for a similiar header parameter: 'If-Modified-Since' in section [14.25]( https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.25)
That seems to be what AWS is basing their implementation on.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5523
## How was this patch tested?
Ran the s3 compatibility tests against aws as well as s3 gateway.
--
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
[GitHub] [ozone] kerneltime commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#discussion_r685522738
##########
File path: hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestMultipartUploadWithCopy.java
##########
@@ -208,6 +218,73 @@ public void testMultipartIfModifiedSince() throws Exception {
}
}
+ // Test operations with 2 datestrings; one valid, the other invalid;
+ // confirm the invalid one is ignored and the valid precondition tested
Review comment:
Thank you for adding this, we should have 12 test cases or more, 4 based on true and false condition for timestamps that are valid, 4 with either one of the time stamps is null and the other true or false, 4 for either timestamp in the future and other time stamp true or false but valid and finally actual bad input.
--
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
[GitHub] [ozone] kerneltime commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#discussion_r685522738
##########
File path: hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestMultipartUploadWithCopy.java
##########
@@ -208,6 +218,73 @@ public void testMultipartIfModifiedSince() throws Exception {
}
}
+ // Test operations with 2 datestrings; one valid, the other invalid;
+ // confirm the invalid one is ignored and the valid precondition tested
Review comment:
Thank you for adding this, we should have 12 test cases or more, 4 based on true and false condition for timestamps that are valid, 4 with either one of the time stamps is null and the other true or false, 4 for ether timestamp in the future and other time stamp true or false but valid and finally actual bad input.
--
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
[GitHub] [ozone] adoroszlai commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by 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
[GitHub] [ozone] kerneltime commented on pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#issuecomment-914717555
The changes looks complete now, have you tried running this against AWS S3 to see if the responses match in all 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
[GitHub] [ozone] GeorgeJahad commented on pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#issuecomment-914776218
Hmmm, I like that enum approach a lot. Let me play with it a bit, and make sure I don't see any problems.
(Also, thank you for typing it all out. Totally beyond the call of duty.)
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#issuecomment-914781845
> The changes looks complete now, have you tried running this against
> AWS S3 to see if the responses match in all cases?
yes
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#issuecomment-928387860
Thanks @kerneltime!
--
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
[GitHub] [ozone] kerneltime commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#discussion_r703923913
##########
File path: hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestMultipartUploadWithCopy.java
##########
@@ -156,56 +172,104 @@ public void testMultipart() throws Exception {
}
}
+ /* The next two tests excercise all the combinations of modification times.
Review comment:
```suggestion
/* The next two tests exercise all the combinations of modification times.
```
--
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
[GitHub] [ozone] GeorgeJahad commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on a change in pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#discussion_r685561957
##########
File path: hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestMultipartUploadWithCopy.java
##########
@@ -208,6 +218,73 @@ public void testMultipartIfModifiedSince() throws Exception {
}
}
+ // Test operations with 2 datestrings; one valid, the other invalid;
+ // confirm the invalid one is ignored and the valid precondition tested
Review comment:
Will do, Thanks @kerneltime!
--
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
[GitHub] [ozone] adoroszlai merged pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #2509:
URL: https://github.com/apache/ozone/pull/2509
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#issuecomment-928387860
Thanks @kerneltime!
--
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
[GitHub] [ozone] kerneltime commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#discussion_r685420815
##########
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 {
+ // If the return is empty, the precondition should be ignored
+ private static OptionalLong parseOzoneDate(String ozoneDateStr)
Review comment:
```suggestion
private static OptionalLong parseAndValidateDate(String ozoneDateStr)
```
--
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
[GitHub] [ozone] GeorgeJahad commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
GeorgeJahad commented on a change in pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#discussion_r692374266
##########
File path: hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestMultipartUploadWithCopy.java
##########
@@ -208,6 +218,73 @@ public void testMultipartIfModifiedSince() throws Exception {
}
}
+ // Test operations with 2 datestrings; one valid, the other invalid;
+ // confirm the invalid one is ignored and the valid precondition tested
Review comment:
hey @kerneltime I've added the tests you suggested to s3/endpoint/TestMultipartUploadWithCopy.java. There ended up being 25 different combinations total. I had to restructure the tests a bit to reduce the repeated boilerplate. Hopefully those changes won't be too hard to follow.
Here are just the changes since your review, if that makes it easier:
https://github.com/GeorgeJahad/ozone/compare/3e7edd84a1a7cbf3e0c8a95c05ae971be171ffa1..f567f523bd12f88ff6db96c5cfdfa6d0a853399a#diff-bd6abd262ed8e24eaeea5cb0e7c2a1a128411b284f9f27dae1ad47be6717a940
Thanks again!
--
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
[GitHub] [ozone] kerneltime commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#discussion_r707480031
##########
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:
Should this check the ERROR_CODE explicitly?
--
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
[GitHub] [ozone] GeorgeJahad commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [ozone] kerneltime commented on pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#issuecomment-914743987
I would recommend something like this to better explain all the variations and possible outcomes.
```
// 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_FUTURE_TS_UNMODIFIED_SINCE_FUTURE_TS(afterSourceKeyModificationTimeStr, afterSourceKeyModificationTimeStr, S3ErrorTable.PRECOND_FAILED.getCode()),
MODIFIED_SINCE_FUTURE_TS_UNMODIFIED_SINCE_PAST_TS(afterSourceKeyModificationTimeStr, beforeSourceKeyModificationTimeStr, S3ErrorTable.PRECOND_FAILED.getCode()),
MODIFIED_SINCE_FUTURE_TS_UNMODIFIED_SINCE_NULL(afterSourceKeyModificationTimeStr, null, S3ErrorTable.PRECOND_FAILED.getCode()),
MODIFIED_SINCE_FUTURE_TS_UNMODIFIED_SINCE_ABSOLUTE_FUTURE(afterSourceKeyModificationTimeStr, futureTimeStr, S3ErrorTable.PRECOND_FAILED.getCode()),
MODIFIED_SINCE_FUTURE_TS_UNMODIFIED_SINCE_UNPARSABLE_TS(afterSourceKeyModificationTimeStr, UNPARSABLE_TIME_STR, S3ErrorTable.PRECOND_FAILED.getCode()),
MODIFIED_SINCE_PAST_TS_UNMODIFIED_SINCE_FUTURE_TS(beforeSourceKeyModificationTimeStr, afterSourceKeyModificationTimeStr, null),
MODIFIED_SINCE_PAST_TS_UNMODIFIED_SINCE_PAST_TS(beforeSourceKeyModificationTimeStr, beforeSourceKeyModificationTimeStr, S3ErrorTable.PRECOND_FAILED.getCode()),
MODIFIED_SINCE_PAST_TS_UNMODIFIED_SINCE_NULL(beforeSourceKeyModificationTimeStr, null, null),
MODIFIED_SINCE_PAST_TS_UNMOFIFIED_SINCE_ABSOLUTE_FUTURE(beforeSourceKeyModificationTimeStr,futureTimeStr, null),
MODIFIED_SINCE_PAST_TS_UNMODIFIED_SINCE_UNPARSABLE_TS(beforeSourceKeyModificationTimeStr, UNPARSABLE_TIME_STR, null),
MODIFIED_SINCE_NULL_TS_UNMODIFIED_SINCE_FUTURE_TS(null, afterSourceKeyModificationTimeStr, null),
MODIFIED_SINCE_NULL_TS_UNMODIFIED_SINCE_PAST_TS(null, beforeSourceKeyModificationTimeStr, S3ErrorTable.PRECOND_FAILED.getCode()),
MODIFIED_SINCE_NULL_TS_UNMODIFIED_SINCE_NULL_TS(null, null, null),
MODIFIED_SINCE_NULL_TS_UNMODIFIED_SINCE_ABSOLUTE_TS(null, futureTimeStr, null),
MODIFIED_SINCE_NULL_TS_UNMODIFIED_SINCE_UNPARSABLE_TS(null, UNPARSABLE_TIME_STR, null),
MODIFIED_SINCE_UNPARSABLE_TS_UNMODIFIED_SINCE_FUTURE_TS(UNPARSABLE_TIME_STR, afterSourceKeyModificationTimeStr, null),
MODIFIED_SINCE_UNPARSABLE_TS_UNMODIFIED_SINCE_PAST_TS(UNPARSABLE_TIME_STR, beforeSourceKeyModificationTimeStr, S3ErrorTable.PRECOND_FAILED.getCode()),
MODIFIED_SINCE_UNPARSABLE_TS_UNMODIFIED_SINCE_NULL_TS(UNPARSABLE_TIME_STR, null, null),
MODIFIED_SINCE_UNPARSABLE_TS_UNMODIFIED_SINCE_ABSOLUTE_TS(UNPARSABLE_TIME_STR, futureTimeStr, null),
MODIFIED_SINCE_UNPARSABLE_TS_UNMODIFIED_SINCE_UNPARSABLE_TS(UNPARSABLE_TIME_STR, UNPARSABLE_TIME_STR, null),
MODIFIED_SINCE_ABS_FUTURE_TS_UNMODIFIED_SINCE_FUTURE_TS(futureTimeStr, afterSourceKeyModificationTimeStr, null),
MODIFIED_SINCE_ABS_FUTURE_TS_UNMODIFIED_SINCE_PAST_TS(futureTimeStr, beforeSourceKeyModificationTimeStr, S3ErrorTable.PRECOND_FAILED.getCode()),
MODIFIED_SINCE_ABS_FUTURE_TS_UNMODIFIED_SINCE_NULL_TS(futureTimeStr, null, null),
MODIFIED_SINCE_ABS_FUTURE_TS_UNMODIFIED_SINCE_ABSOLUTE_TS(futureTimeStr, futureTimeStr, null),
MODIFIED_SINCE_ABS_FUTURE_TS_UNMODIFIED_SINCE_UNPARSABLE_TS(futureTimeStr, UNPARSABLE_TIME_STR, null);
private String modifiedTimestamp;
private String unmodifiedTimestamp;
private String errorCode;
CopyIfTimestampTestCase(String modifiedTimestamp, String unmodifiedTimestamp, String errorCode) {
this.modifiedTimestamp = modifiedTimestamp;
this.unmodifiedTimestamp = unmodifiedTimestamp;
this.errorCode = errorCode;
}
@Override
public String toString() {
return " Modified:" + this.modifiedTimestamp
+ " Unmodified:" + this.unmodifiedTimestamp
+ " ErrorCode:" + this.errorCode;
}
}
@Test
public void testMultipartTSHeaders() throws Exception {
for (CopyIfTimestampTestCase t : CopyIfTimestampTestCase.values() ) {
try {
uploadPartWithCopy(t.modifiedTimestamp, t.unmodifiedTimestamp);
if (t.errorCode != null) {
fail("Fail test:" + t);
}
} catch (OS3Exception ex) {
if (t.errorCode == null) {
fail("Failed test:" + t );
}
}
}
}
```
--
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
[GitHub] [ozone] adoroszlai commented on pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#issuecomment-931363301
Thanks @GeorgeJahad for the patch, @kerneltime for the review.
--
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
[GitHub] [ozone] adoroszlai commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by 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
[GitHub] [ozone] kerneltime commented on a change in pull request #2509: HDDS-5523. Fix multipart upload failure in s3 compatibility tests
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2509:
URL: https://github.com/apache/ozone/pull/2509#discussion_r685420665
##########
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 {
+ // If the return is empty, the precondition should be ignored
Review comment:
Can you please update this comment and others in the method.
```suggestion
// Parses date string and return long representation. Returns an empty if DateStr is null or invalid. Date in the future are considered invalid.
```
--
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