added ListObjectsV2 call #64

ExpandingMan · 2019-12-04T19:21:34Z

This adds the ListObjectsV2 call. As far as I can tell, this is the only way to actually list all objects in a bucket. No matter how I try, the older version never lists more than 1000 objects for me.

Right now the way I have this written gives lots of code duplication. I left it this way for now because merging s3_list_objects and s3_list_objects_v2 was a little bit messy. As I see it we have 3 options:

Leave it as is (two completely separate functions).
Merge the two functions and handle all possible keys in one function.
Deprecate s3_list_objects in favor of the new version. I can't see any reason not to do this as the older version seems like a subset of the new one.

bkamins · 2019-12-04T19:36:34Z

CC @pszufe

mattBrzezinski · 2019-12-04T21:44:46Z

No matter how I try, the older version never lists more than 1000 objects for me.

ListObjects is limited to 1,000 objects.

Deprecate s3_list_objects in favor of the new version. I can't see any reason not to do this as the older version seems like a subset of the new one.

I think this makes the most sense.

mattBrzezinski

Just did a first pass on this CR. I'm in favour of just marking list_objects as deprecated and getting people to use v2.

Also, could you bump the version number in the Project.toml, and add some testing for this?

mattBrzezinski · 2019-12-04T21:48:35Z

src/AWSS3.jl

+            if delimiter != ""
+                q["delimiter"] = delimiter
+            end
+            if contoken ≠ ""


For consistency we should probably keep using != instead of ≠

mattBrzezinski · 2019-12-04T21:56:16Z

src/AWSS3.jl

+                # Add each object from the response and update our object count / marker
+                if haskey(r, "Contents")
+                    l = isa(r["Contents"], Vector) ? r["Contents"] : [r["Contents"]]
+                    for object in l


Instead of iterating over these objects, could we not just put! the vector into the channel? (Not sure if this is possible).

And then just do num_objects += size(l)[1]

Wouldn't the xml_dict() function still need to be applied to each element in that vector before put!-ing the vector to the channel?

pszufe · 2019-12-04T22:19:10Z

In production systems an S3 bucket can contain millions of items and S3 has no directory structure (this what you see as directories are in fact name prefixes so there is not such thing as directory index).
Maybe since you have a chance yo use a new AWS API it would be also a good idea to update the AWSS3 API?
Perhaps, s3_list_objects_v2 could return some ResultSet obejct and then one can call getdata(resultset) and next!(resultset). Otherwise for large scale system this API will be not usable anyway (listing 1million objects can take very considerable amount of time while a typical large scale scenario is to list objects and asynchronously launch jobs for processing them).

I just noticed that you have a Channel in the new code so disregard the last comment. Paginating via a Channel seems to be more beautiful. so disregard the last paragraph. However, still tests for buckets with more than 1000 items are required.

ExpandingMan · 2019-12-05T15:38:11Z

Note that there is (and has been) an option for limiting the number of items returned, so I'd assume it would work fine as is.

Is there a consensus on deprecating the older ListObjects in favor of this new ListObjectsV2?

If so I will eliminate the old function and try to make sure we don't lose any functionality in the new one (might have to add a couple of keys, I will check).

pszufe · 2019-12-05T16:13:45Z

The AWS S# REST API docs reads (https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html):

We recommend that you use this revised API for application development. For backward compatibility, Amazon S3 continues to support the prior version of this API, ListObjects.

So it seems to be reasonable to depreciate the old version.

mattBrzezinski · 2019-12-05T16:46:38Z

I am in favour of adding the @deprecated tag to the ListObjects function, introducing the ListObjectsV2 call and releasing a patch rather than removing the ListObjects function and doing a major release.

ExpandingMan · 2019-12-11T15:52:45Z

Any concensus about what should be done here so I can update the PR? Are we ok with deprecating the old version?

s2maki · 2019-12-13T18:53:53Z

The only reason I can think of for keeping both is that the pagination arguments are different between calls. ListObjects uses Marker, while V2 uses ContinuationToken. I use ListObjects, but I expect my object count to be small (a couple dozen). Anything more than that, and I find I have to maintain my index another way anyhow.

mattBrzezinski · 2020-07-31T17:31:14Z

X-posting a similar merge request from @pschmied back in here:

#94

pschmied · 2020-08-03T16:27:51Z

The only reason I can think of for keeping both is that the pagination arguments are different between calls. ListObjects uses Marker, while V2 uses ContinuationToken. I use ListObjects, but I expect my object count to be small (a couple dozen). Anything more than that, and I find I have to maintain my index another way anyhow.

In the V1 ListObjects API, the "marker" parameter causes the API to return objects that follow the marker lexicographically:

https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjects.html#API_ListObjects_RequestSyntax

In the ListObjectsV2 API, the "start-after" parameter causes the API to return objects that follow the start-after parameter lexicographically:

https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListObjectsV2.html#API_ListObjectsV2_RequestSyntax

My feeling is that exposing the "start-after" parameter in the s3_list_objects_v2 function would allow anyone relying on the old "marker" parameter to do what they need to do. Thus you could deprecate the old ListObjects / s3_list_objects. Do others agree?

pschmied

By adding the start-after query parameter, this should allow people to achieve with the ListObjectsV2 API what they were able to do with the deprecated ListObjects API.

pschmied · 2020-08-04T17:59:53Z

src/AWSS3.jl


+"""
+    s3_list_objects_v2([::AWSConfig], bucket, [path_prefix]; delimiter="/", max_items=1000)


Suggested change

s3_list_objects_v2([::AWSConfig], bucket, [path_prefix]; delimiter="/", max_items=1000)

s3_list_objects_v2([::AWSConfig], bucket, [path_prefix]; delimiter="/", start_after="", max_items=1000)

pschmied · 2020-08-04T18:01:46Z

src/AWSS3.jl

+
+This uses the `ListObjectV2` function call.
+"""
+function s3_list_objects_v2(aws::AWSS3.AWSConfig, bucket, path_prefix=""; delimiter="/", max_items=nothing)


Suggested change

function s3_list_objects_v2(aws::AWSS3.AWSConfig, bucket, path_prefix=""; delimiter="/", max_items=nothing)

function s3_list_objects_v2(aws::AWSS3.AWSConfig, bucket, path_prefix=""; delimiter="/", start_after="", max_items=nothing)

pschmied · 2020-08-04T18:03:01Z

src/AWSS3.jl

+            end
+            if contoken ≠ ""
+                q["continuation-token"] = contoken
+            end


Suggested change

end

end

if start_after != ""

q["start-after"] = start_after

end

pschmied · 2020-08-04T18:03:19Z

src/AWSS3.jl

+            if delimiter != ""
+                q["delimiter"] = delimiter
+            end
+            if contoken ≠ ""


Suggested change

if contoken ≠ ""

if contoken != ""

pschmied · 2020-08-11T19:52:54Z

Just bumping here to see if there's anything more I can do to help move this along.

pschmied · 2020-08-12T18:41:52Z

If it is useful, I have collected I think all of the feedback, plus tests into the master branch of my fork: https://github.com/pschmied/AWSS3.jl

mattBrzezinski · 2020-08-13T00:08:20Z

Just bumping here to see if there's anything more I can do to help move this along.

I'm currently quite busy with other work, but at some point I will have time to dedicate to this. If someone else could take the lead on reviewing / giving feedback that'd be greatly appreciated!

ExpandingMan added 2 commits December 4, 2019 14:14

added ListObjectsV2 call

86a49fa

added export of s3_list_objects_v2

013930d

mattBrzezinski self-assigned this Dec 4, 2019

mattBrzezinski reviewed Dec 4, 2019

View reviewed changes

mattBrzezinski mentioned this pull request Jul 31, 2020

Update s3_list_objects() to use ListObjectsV2 #94

Closed

pschmied suggested changes Aug 4, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added ListObjectsV2 call #64

added ListObjectsV2 call #64

ExpandingMan commented Dec 4, 2019

bkamins commented Dec 4, 2019

mattBrzezinski commented Dec 4, 2019

mattBrzezinski left a comment

mattBrzezinski Dec 4, 2019

mattBrzezinski Dec 4, 2019

pschmied Aug 4, 2020

pszufe commented Dec 4, 2019 •

edited

Loading

ExpandingMan commented Dec 5, 2019

pszufe commented Dec 5, 2019

mattBrzezinski commented Dec 5, 2019

ExpandingMan commented Dec 11, 2019

s2maki commented Dec 13, 2019

mattBrzezinski commented Jul 31, 2020

pschmied commented Aug 3, 2020

pschmied left a comment

pschmied Aug 4, 2020

pschmied Aug 4, 2020

pschmied Aug 4, 2020

pschmied Aug 4, 2020

pschmied commented Aug 11, 2020

pschmied commented Aug 12, 2020

mattBrzezinski commented Aug 13, 2020


		"""
		s3_list_objects_v2([::AWSConfig], bucket, [path_prefix]; delimiter="/", max_items=1000)

	function s3_list_objects_v2(aws::AWSS3.AWSConfig, bucket, path_prefix=""; delimiter="/", max_items=nothing)
	function s3_list_objects_v2(aws::AWSS3.AWSConfig, bucket, path_prefix=""; delimiter="/", start_after="", max_items=nothing)

-            end
+            end
+            if start_after != ""
+                q["start-after"] = start_after
+            end

added ListObjectsV2 call #64

Are you sure you want to change the base?

added ListObjectsV2 call #64

Conversation

ExpandingMan commented Dec 4, 2019

bkamins commented Dec 4, 2019

mattBrzezinski commented Dec 4, 2019

mattBrzezinski left a comment

Choose a reason for hiding this comment

mattBrzezinski Dec 4, 2019

Choose a reason for hiding this comment

mattBrzezinski Dec 4, 2019

Choose a reason for hiding this comment

pschmied Aug 4, 2020

Choose a reason for hiding this comment

pszufe commented Dec 4, 2019 • edited Loading

ExpandingMan commented Dec 5, 2019

pszufe commented Dec 5, 2019

mattBrzezinski commented Dec 5, 2019

ExpandingMan commented Dec 11, 2019

s2maki commented Dec 13, 2019

mattBrzezinski commented Jul 31, 2020

pschmied commented Aug 3, 2020

pschmied left a comment

Choose a reason for hiding this comment

pschmied Aug 4, 2020

Choose a reason for hiding this comment

pschmied Aug 4, 2020

Choose a reason for hiding this comment

pschmied Aug 4, 2020

Choose a reason for hiding this comment

pschmied Aug 4, 2020

Choose a reason for hiding this comment

pschmied commented Aug 11, 2020

pschmied commented Aug 12, 2020

mattBrzezinski commented Aug 13, 2020

pszufe commented Dec 4, 2019 •

edited

Loading