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.

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

Ensure _list_bucket uses continuation token for subsequent pages #246

Merged
merged 3 commits into from Jan 17, 2019

Conversation

tcsavage
Copy link
Contributor

@tcsavage tcsavage commented Nov 8, 2018

When trying to s3_iter_bucket a large set of objects (more than 1000), we found we were given the same set of 1000 over and over again, forever.

The issue was that the continuation token to fetch the next page, although it was being captured, was not being given to subsequent invocations of list_objects_v2. This pull request makes sure that if we have a continuation token, that we use it.

Unfortunately, I have been unable to run the test suite but this change should fix the test "test_list_bucket_long" which is currently being skipped because it was taking "too long". Probably because it would run forever.

The continuation token is being captured when each page of keys is
fetched, but it was not used for subsequent requests. This meant that
the same page of 1000 keys would repeat forever.

This commit adds a fix to ensure the continuation token is used if not
None
smart_open/s3.py Outdated Show resolved Hide resolved
@mpenkov
Copy link
Collaborator

mpenkov commented Nov 9, 2018

Thank you for this PR. Does the affected test pass now?

Co-Authored-By: tcsavage <tcsavage@gmail.com>
@tcsavage
Copy link
Contributor Author

tcsavage commented Nov 9, 2018

@mpenkov As I mentioned, I was unable to run the tests on my local machine, but I will remove the skip decorator and see if it passes in Travis.

@@ -391,7 +391,6 @@ def test_list_bucket(self):
expected = ['key_%d' % x for x in range(num_keys)]
self.assertEqual(sorted(keys), sorted(expected))

@unittest.skip('this test takes too long for some unknown reason')
Copy link
Owner

@piskvorky piskvorky Nov 9, 2018

Choose a reason for hiding this comment

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

LOL! Did we catch the error, but commented out its test instead of fixing? :) Who did this?

@tcsavage
Copy link
Contributor Author

tcsavage commented Nov 9, 2018

The build failed, but it looks like a problem with test_old taking too long to run. test_list_bucket_long itself passed just fine. Is test_old known to be a bit flakey?

@tcsavage
Copy link
Contributor Author

@mpenkov @piskvorky The tests pass, could we merge and release before this gets stale?

Copy link
Collaborator

@mpenkov mpenkov left a comment

Choose a reason for hiding this comment

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

@menshikh-iv I think this is good to merge.

@menshikh-iv
Copy link
Contributor

Thank you @tcsavage, congratz with the first contribution 🥇

@menshikh-iv menshikh-iv merged commit e269610 into piskvorky:master Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants