HDDS-12336. Support Prefix, Delimiter, and CommonPrefixes in ListMultipartUploads#9286
HDDS-12336. Support Prefix, Delimiter, and CommonPrefixes in ListMultipartUploads#92860lai0 wants to merge 21 commits intoapache:masterfrom
Conversation
|
@echonesis Please also help take a look. |
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java
Outdated
Show resolved
Hide resolved
.../s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ListMultipartUploadsResult.java
Show resolved
Hide resolved
|
Thanks @echonesis for pointing this out. I'll get on this. |
chungen0126
left a comment
There was a problem hiding this comment.
Thanks @0lai0 for the patch. Left some comments
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for this patch, left some comments.
Few thoughts that I didnt list in the code comments:
- lets add a builder for list multipart uploads response class, and move the complex common prefix extracting logic into it.
- Please add some test case coverage into AWS SDK IT
.../s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ListMultipartUploadsResult.java
Outdated
Show resolved
Hide resolved
| new ListMultipartUploadsResult.Upload( | ||
| upload.getKeyName(), | ||
| String prevDir = null; | ||
| for (OzoneMultipartUpload upload : ozoneMultipartUploadList.getUploads()) { |
There was a problem hiding this comment.
These extract common prefix should only happen if delimiter has value
| String prevDir = null; | ||
| for (OzoneMultipartUpload upload : ozoneMultipartUploadList.getUploads()) { | ||
| String keyName = upload.getKeyName(); | ||
| if (bucket.getBucketLayout().isFileSystemOptimized() && |
There was a problem hiding this comment.
Please could you add some comment to explain the logic here? thanks very much!
| if (!addedAsPrefix) { | ||
| result.addUpload(new ListMultipartUploadsResult.Upload( |
There was a problem hiding this comment.
Is this intentional? some records return from OM would disappear
There was a problem hiding this comment.
Yes, When delimiter is provided, uploads sharing a common prefix are collapsed into CommonPrefixes instead of appearing individually in Uploads. So I think it would not disappear.
There was a problem hiding this comment.
But I wasn't 100% sure about this part. Does this implementation look correct ? If I still get incorrect way , could you provide some guidance? Thank you so much.
|
Thanks @peterxcli . I'll get on this. |
|
Thanks @0lai0 for the patch, I'll take a look soon. Meanwhile, please kindly refer to the https://issues.apache.org/jira/browse/HDDS-12882 comment regarding the case where Delimiter is specified (i.e. how to derive CommonPrefixes and Contents). |
|
Thanks @ivandika3 for pointing this out. I'll look into this. |
|
@0lai0 Thanks, please also add similar tests in |
|
Thank you. I'll do some research on how to approach this. |
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
| if (keyName.length() < normalizedPrefix.length()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This check is redundant when getBucketLayout().isFileSystemOptimized() is true
(covered by L450's startsWith check).
Should the prefix filtering apply to all bucket layouts?
| && currentDirName.equals(prevDir)) { | ||
| lastProcessedKey = keyName; | ||
| lastProcessedUploadId = upload.getUploadId(); | ||
| continue; |
There was a problem hiding this comment.
processedUploads won't be added in this condition.
Is that what we want?
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
| final String aclMarker = queryParams().get(QueryParams.ACL); | ||
| if (aclMarker != null) { | ||
| s3GAction = S3GAction.GET_ACL; | ||
| S3BucketAcl result = getAcl(bucketName); | ||
| getMetrics().updateGetAclSuccessStats(startNanos); | ||
| auditReadSuccess(s3GAction); | ||
| return Response.ok(result, MediaType.APPLICATION_XML_TYPE).build(); | ||
| } |
There was a problem hiding this comment.
Please don't restore this, it was moved to BucketAclHandler in HDDS-14360.
| String delimiter, | ||
| String encodingType, |
There was a problem hiding this comment.
listMultipartUploads call does not pass these new arguments, causing compile error.
| AUDIT.logReadSuccess(buildAuditMessageForSuccess(s3GAction, | ||
| getAuditParameters())); |
There was a problem hiding this comment.
Please keep simplified auditReadSuccess(s3GAction) call.
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
What changes were proposed in this pull request?
To enhance the AWS behavior of the ListMultipartUploads API in Ozone S3 Gateway:
delimiterandencoding-typequery parameters, and includeDelimiter,EncodingType, andCommonPrefixesin the response.ListMultipartUploadsResultnow wrapsPrefixandDelimiterwithEncodingTypeObject, and supportsCommonPrefixes.ListObjects/V2method, correctly generating common prefixes.What is the link to the Apache JIRA
HDDS-12336