Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add arg builder to listIncompleteUpload API #937

Merged
merged 7 commits into from
Jun 12, 2020
Merged

Add arg builder to listIncompleteUpload API #937

merged 7 commits into from
Jun 12, 2020

Conversation

sinhaashish
Copy link
Contributor

@sinhaashish sinhaashish commented May 15, 2020 •
edited
Loading

Add arg builder to listIncompleteUpload

@sinhaashish sinhaashish force-pushed the listIncompleteuploads branch from 2530011 to 9242386 Compare May 15, 2020 08:11
@kannappanr kannappanr requested a review from anjalshireesh May 15, 2020 18:02
@sinhaashish sinhaashish force-pushed the listIncompleteuploads branch from 1d5df85 to 5d270bc Compare May 18, 2020 10:03
@sinhaashish sinhaashish changed the title ListIncompleteUpload using builder Add arg builder to listIncompleteUpload API May 18, 2020
@sinhaashish sinhaashish force-pushed the listIncompleteuploads branch 6 times, most recently from dc4684e to 6808bb8 Compare May 20, 2020 05:27
@sinhaashish sinhaashish force-pushed the listIncompleteuploads branch from 877dff7 to 7363336 Compare May 20, 2020 09:30
balamurugana
balamurugana reviewed May 21, 2020
View reviewed changes
api/src/main/java/io/minio/ListIncompleteUploadsArgs.java
package io.minio;

/** Argument class of @see #listIncompleteUploads(ListIncompleteUploadsArgs args). */
public class ListIncompleteUploadsArgs extends BucketArgs {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add all parameters like listObjects()? Similarly you could check reusing/modifying existing objectIterator class to accommodate this listing as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this not feasible as there are two different API's expecting different query parameters.
List Incomplete upload : https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListMultipartUploads.html

GET /?uploads&Delimiter=Delimiter&EncodingType=EncodingType&KeyMarker=KeyMarker&MaxUploads=MaxUploads&Prefix=Prefix&UploadIdMarker=UploadIdMarker HTTP/1.1
Host: Bucket.s3.amazonaws.com

List Objects : https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html

GET /?list-type=2&ContinuationToken=ContinuationToken&Delimiter=Delimiter&EncodingType=EncodingType&FetchOwner=FetchOwner&MaxKeys=MaxKeys&Prefix=Prefix&StartAfter=StartAfter HTTP/1.1
Host: Bucket.s3.amazonaws.com
x-amz-request-payer: RequestPayer

As far as using object Iterator is concerned it implements Iterator of Item type, (ObjectIterator implements Iterator<Result<Item>> ) while list incompleteUpload needs Iterator of type Upload Iterator<Result<Upload>> . Item and Upload the the different response classes for these api.
IMO listObjects and listIncompleteObjects being separate is correct.

@anjalshireesh anjalshireesh self-requested a review May 21, 2020 08:10
anjalshireesh
anjalshireesh previously approved these changes May 21, 2020
View reviewed changes
Copy link
Contributor

@anjalshireesh anjalshireesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sinhaashish sinhaashish force-pushed the listIncompleteuploads branch 3 times, most recently from dc02069 to 517aa48 Compare June 9, 2020 03:59
@sinhaashish sinhaashish force-pushed the listIncompleteuploads branch from 517aa48 to 32d7d65 Compare June 10, 2020 09:49
@sinhaashish sinhaashish force-pushed the listIncompleteuploads branch from 32d7d65 to 9862e92 Compare June 10, 2020 10:01
anjalshireesh
anjalshireesh requested changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@anjalshireesh anjalshireesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of minor comments apart from some other comments which have already been added by Bala.

@sinhaashish sinhaashish force-pushed the listIncompleteuploads branch from 4cd989b to 7bb2361 Compare June 10, 2020 16:42
balamurugana
balamurugana approved these changes Jun 12, 2020
View reviewed changes
Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

anjalshireesh
anjalshireesh approved these changes Jun 12, 2020
View reviewed changes
Copy link
Contributor

@anjalshireesh anjalshireesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@balamurugana balamurugana merged commit 7a5cae1 into minio:master Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anjalshireesh anjalshireesh anjalshireesh approved these changes

@balamurugana balamurugana balamurugana approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sinhaashish @anjalshireesh @balamurugana