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

service/s3/s3manager: Add DownloadStrategy for buffering downloaded parts #2823

Merged
merged 2 commits into from
Sep 25, 2019
Merged

service/s3/s3manager: Add DownloadStrategy for buffering downloaded parts #2823

merged 2 commits into from
Sep 25, 2019

Conversation

skmcgrail
Copy link
Member

Implements a buffering strategy for the download S3 manager. This is similar to the upload strategy being reviewed as part of #2792

@skmcgrail skmcgrail added the needs-review This issue or pull request needs review from a core team member. label Sep 10, 2019
@skmcgrail skmcgrail force-pushed the s3manager/downloader branch 15 times, most recently from 9049c8b to 704b53e Compare September 17, 2019 16:11
jasdel
jasdel suggested changes Sep 18, 2019
View reviewed changes
Copy link
Contributor

@jasdel jasdel left a comment

Choose a reason for hiding this comment

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

Looks good, just a few comments

awstesting/integration/performance/s3DownloadManager/endless_reader.go Outdated Show resolved Hide resolved
awstesting/integration/performance/s3DownloadManager/discard.go Outdated Show resolved Hide resolved
awstesting/integration/constants.go Outdated Show resolved Hide resolved
service/s3/s3manager/download.go Outdated Show resolved Hide resolved
service/s3/s3manager/download.go Show resolved Hide resolved
service/s3/s3manager/download.go Outdated Show resolved Hide resolved
service/s3/s3manager/download_test.go Outdated Show resolved Hide resolved
@skmcgrail skmcgrail force-pushed the s3manager/downloader branch 3 times, most recently from 60065de to 97b588b Compare September 19, 2019 23:42
jasdel
jasdel suggested changes Sep 20, 2019
View reviewed changes
Copy link
Contributor

@jasdel jasdel left a comment

Choose a reason for hiding this comment

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

Thanks for making these updates. Changes look good, just a couple more suggestions.

awstesting/integration/performance/s3DownloadManager/main_test.go Outdated Show resolved Hide resolved
internal/sdkconstants/byte.go Outdated Show resolved Hide resolved
service/s3/s3manager/download.go Show resolved Hide resolved
service/s3/s3manager/download.go Outdated Show resolved Hide resolved
service/s3/s3manager/download.go Show resolved Hide resolved
service/s3/s3manager/download_test.go Outdated Show resolved Hide resolved
jasdel
jasdel suggested changes Sep 24, 2019
View reviewed changes
Copy link
Contributor

@jasdel jasdel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, minor goimports formatting.

also adding changelog release notes.

service/s3/s3manager/download_test.go Outdated Show resolved Hide resolved
@skmcgrail
Copy link
Member Author

Fixed improper import ordering for the sdkio package, and updated changelog.

jasdel
jasdel reviewed Sep 25, 2019
View reviewed changes
CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
@skmcgrail skmcgrail removed the needs-review This issue or pull request needs review from a core team member. label Sep 25, 2019
@skmcgrail skmcgrail merged commit 4cfae4a into aws:master Sep 25, 2019
@skmcgrail skmcgrail deleted the s3manager/downloader branch September 25, 2019 23:15
@aws-sdk-go-automation aws-sdk-go-automation mentioned this pull request Sep 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasdel jasdel jasdel 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.
2 participants
@skmcgrail @jasdel