-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
More efficient s3 paging #2780
More efficient s3 paging #2780
Conversation
The appendIfMissing function is used by several physical backends so move the function to a common file.
The ListObjectsV2 API allows you to specify a folder delimiter when listing objects in a bucket. This makes S3 group the folders together and only list the objects in the immediate prefix. For prefixes with a large number of objects underneath, this can speed up querying of intermediate paths significantly.
This allows you to run the S3 integration tests with the ~/.aws/credentials file and AWS_PROFILE environment variable.
I realise I forgot to explain the credentials handling changes in the test. Previously the AWS access keys had to be in the environment. Now they can be in the environment or in the Also, here's the acceptance test output:
|
physical/s3_test.go
Outdated
if os.Getenv("AWS_ACCESS_KEY_ID") == "" || os.Getenv("AWS_SECRET_ACCESS_KEY") == "" { | ||
t.SkipNow() | ||
} | ||
creds := credentials.NewChainCredentials( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at using helper/awsutil
for this as it has unified logic.
physical/common.go
Outdated
@@ -0,0 +1,10 @@ | |||
package physical | |||
|
|||
func appendIfMissing(slice []string, i string) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than do this, can you make this a method in helper/strutil
? There's an existing method to do the search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I totally missed the helper/strutil package when I did this originally. This is much better.
physical/s3_test.go
Outdated
if os.Getenv("AWS_ACCESS_KEY_ID") == "" || os.Getenv("AWS_SECRET_ACCESS_KEY") == "" { | ||
t.SkipNow() | ||
} | ||
credsConfig := &awsutil.CredentialsConfig{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These environment variables were guarding the test from running on a system without credentials. The travis tests are failing because these tests are running now. I don't think the below error check is working
This PR should speed up S3 backends that have a lot of subobjects. Instead of grouping folders on the client side, it asks S3 to do the work before returning the response.