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

fix(s3): transfers are now single-threaded #2447

Merged
merged 3 commits into from Apr 19, 2021
Merged

Conversation

raphkim
Copy link
Contributor

@raphkim raphkim commented Apr 15, 2021

Issue #, if available:
#1947

Description of changes:
Default behavior for transfer utility was to use thread pool size = # available processors. This allowed multipart upload to be executed in parallel for a minor performance boost. However, we found that this was causing certain upload part requests to be prioritized over others, which in turn timed out the requests that were not being prioritized.

To increase the stability of large file uploads, this PR aims to force single-threaded multipart uploads by default (parts will be uploaded serially).

Main task executor to be used for downloads and single-part uploads will still be multi-threaded so that small transfers are not blocked by large file uploads.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@raphkim raphkim requested review from richardmcclellan and a team April 15, 2021 22:48
@richardmcclellan
Copy link
Contributor

Could we just change the default pool size to 1, defined here instead? That way, we won't break the override.

@raphkim
Copy link
Contributor Author

raphkim commented Apr 16, 2021

Main task executor was left untouched intentionally so that one large file upload does not block other transfers completely. Otherwise, if a user wants to upload a 1GB file while downloading 3 small files (5KB each) from S3, then the downloads will not start until upload is completed.

@richardmcclellan
Copy link
Contributor

Could we just change the default pool size to 1, defined here instead? That way, we won't break the override.

@raphkim and I chatted offline about this. Changing the default pool size to 1 would impact both the main executor, and the part executor. We do want to serialize the part executor tasks, because the performance benefit with large (5MB) payloads is minimal, at the expense of reliability. However, that's not true for small payloads, because there is performance benefit in parallelizing the initial latency for each request. For example, if downloading multiple small files, parallelizing will likely help performance. And, timeouts are much less likely, since each task finishes quickly, so reliability is not as big of an issue.

So, the change in this PR hard-codes the part executor pool size to 1, but still lets customers customize the size of the main executor thread pool (via setTransferThreadPoolSize(...))

@raphkim raphkim merged commit d328875 into main Apr 19, 2021
@raphkim raphkim deleted the single-thread-transfer branch April 19, 2021 19:46
div5yesh added a commit that referenced this pull request Jun 30, 2021
* feat(aws-android-sdk-cognitoidentityprovider): support custom endpoint

* feat(aws-android-sdk-cognitoidentityprovider): support custom endpoint unit tests

* feat(aws-android-sdk-cognitoidentityprovider): support custom endpoint unit tests

* feat(aws-android-sdk-machinelearning): update models to latest (#2407)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* feat(aws-android-sdk-iot): update models to latest (#2408)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* feat(aws-android-sdk-comprehend): update models to latest (#2409)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* feat(aws-android-sdk-transcribe): update models to latest (#2410)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* feat(aws-android-sdk-lex): update models to latest (#2413)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* chore(lex): update service name for lex runtime (#2424)

* feat(aws-android-sdk-kinesisvideo-archivedmedia): update models to latest (#2422)

* Revert "feat(aws-android-sdk-cognitoidentityprovider): support custom endpoint" (#2425)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* release: 2.22.6 (#2426)

* fix(mobile-client): missing optional dependency warning removed (#2427)

* fix(mobile-client): missing optional dependency warning removed

* make comment more descriptive

* chore: add fastlane scripts for release automation (#2428)

* fix: change protocol for github import (#2429)

* fix(s3): remove eTag validation logic (#2419)

* chore(build): use in-memory key in CI (#2449)

* change the time offset precision from int to long (#2448)

**Notes:**

The clockskew auto-correct logic in the SDK relies on the `int`
primitive type when calculating the offset. When the offset is converted
from milliseconds to days, the ms represented as an `int` have the
boundaries as -24 and +24 days. Changing it to long (64-bit precision)
fixes the limit.

* fix(s3): force upload part tasks to be serial (#2447)

* feat(aws-android-sdk-core): update models to latest (#2445)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* release: AWS SDK for Android 2.22.7 (#2451)

* release: AWS SDK for Android 2.23.0

* Update CHANGELOG.md

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* Update CHANGELOG.md

* Update gradle.properties

* Update CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>
Co-authored-by: Chang Xu <42978935+changxu0306@users.noreply.github.com>
Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* "feat(aws-android-sdk-cognitoidentityprovider): support custom endpoint" (#2455)

* fix(pinpoint): add campaign attributes to push events (#2458)

* release: AWS SDK for Android 2.23.0 (#2459)

* release: AWS SDK for Android 2.22.8

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update gradle.properties

Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>
Co-authored-by: Chang Xu <42978935+changxu0306@users.noreply.github.com>

* feat(aws-android-sdk-sns): update models to latest (#2461)

* feat(aws-android-sdk-cognitoidentityprovider): update models to latest (#2456)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* chore(build): set region in circleci script (#2467)

* fix: launch hosted-ui sign-out using custom tabs manager (#2472)

* feat(mobile-client): hosted-ui auth response handler is now built into redirect activity (#2473)

* feat(mobile-client): auth response handler is now built into redirect activity

* add javadocs for redirect activities

* add signout latch conditionally

* add no history flag to auth signout flow

* feat(aws-android-sdk-connect): update models to latest (#2469)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-transcribe): update models to latest (#2476)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-rekognition): update models to latest (#2487)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-iot): update models to latest (#2490)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>
Co-authored-by: Rafael Juliano <rjjulian@amazon.com>

* feat(aws-android-sdk-location): update models to latest (#2494)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-sns): update models to latest (#2496)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-polly): update models to latest (#2497)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* chore(sts): add support for regionalizing sts client (#2493)

* feat(sts): add support for regionalizing sts client

* feat(aws-android-sdk-mobile-client): adds signature with user attributes in confirmSignIn (#2492)

* feat(aws-android-sdk-mobile-client): adds signature with user attributes in confirmSignIn

* code review suggestion

Co-authored-by: Noyes <dnnoyes@f8ffc25e9e15.ant.amazon.com>

* release: AWS SDK for Android 2.24.0 (#2500)

* release: AWS SDK for Android 2.24.0

* Reword the changelog

* include instruction for applying fix

Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>
Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* fix(aws-android-sdk-lex): prioritize custom lex signer for all regions (#2506)

* fix(aws-android-sdk-lex): prioritize custom lex signer for all regions

* add tests

* fix(mobileclient): Honor auth flow setting from config (#2499)

* fix(mobileclient): Honor auth flow setting from config

* PR feedback

* fix(aws-android-sdk-polly): use correct SignerConfig in all regions (#2505)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-cognitoidentityprovider): update models to latest (#2510)

* release: AWS SDK for Android 2.25.0 (#2512)

Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>

* chore(docs): releases not pushed to S3 anymore (#2514)

* fix(aws-android-sdk-s3): implement retry mechanism for upload part (#2504)

* implement retry mechanism for upload part

* reduce backoff time and max attempts

* lgtm warning

* feat(aws-android-sdk-connect): update models to latest (#2516)

* feat(aws-android-sdk-kms): update models to latest (#2518)

* release: AWS SDK for Android 2.26.0 (#2525)

Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>

* feat(aws-android-sdk-connect): update models to latest (#2526)

Co-authored-by: Abhash Kumar Singh <abhashs@amazon.com>
Co-authored-by: Abhash Kumar Singh <thisisabhash@gmail.com>
Co-authored-by: Jameson Williams <jhwill@amazon.com>
Co-authored-by: AWS Mobile SDK Team <46607340+awsmobilesdk@users.noreply.github.com>
Co-authored-by: Richard McClellan <ricmccle@amazon.com>
Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>
Co-authored-by: Rafael Juliano <rjjulian@amazon.com>
Co-authored-by: Daniel Rochetti <daniel.rochetti@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>
Co-authored-by: Chang Xu <42978935+changxu0306@users.noreply.github.com>
Co-authored-by: Dustin Noyes <dustin.noyes.dev@gmail.com>
Co-authored-by: Noyes <dnnoyes@f8ffc25e9e15.ant.amazon.com>
Co-authored-by: tllauda <85560392+tllauda@users.noreply.github.com>
div5yesh added a commit that referenced this pull request Jul 30, 2021
* feature: user pool token revocation model updates

* feature: revoke tokens if invalidate tokens specified

* Token revocation (#2532)

* feat(aws-android-sdk-cognitoidentityprovider): support custom endpoint

* feat(aws-android-sdk-cognitoidentityprovider): support custom endpoint unit tests

* feat(aws-android-sdk-cognitoidentityprovider): support custom endpoint unit tests

* feat(aws-android-sdk-machinelearning): update models to latest (#2407)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* feat(aws-android-sdk-iot): update models to latest (#2408)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* feat(aws-android-sdk-comprehend): update models to latest (#2409)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* feat(aws-android-sdk-transcribe): update models to latest (#2410)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* feat(aws-android-sdk-lex): update models to latest (#2413)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* chore(lex): update service name for lex runtime (#2424)

* feat(aws-android-sdk-kinesisvideo-archivedmedia): update models to latest (#2422)

* Revert "feat(aws-android-sdk-cognitoidentityprovider): support custom endpoint" (#2425)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* release: 2.22.6 (#2426)

* fix(mobile-client): missing optional dependency warning removed (#2427)

* fix(mobile-client): missing optional dependency warning removed

* make comment more descriptive

* chore: add fastlane scripts for release automation (#2428)

* fix: change protocol for github import (#2429)

* fix(s3): remove eTag validation logic (#2419)

* chore(build): use in-memory key in CI (#2449)

* change the time offset precision from int to long (#2448)

**Notes:**

The clockskew auto-correct logic in the SDK relies on the `int`
primitive type when calculating the offset. When the offset is converted
from milliseconds to days, the ms represented as an `int` have the
boundaries as -24 and +24 days. Changing it to long (64-bit precision)
fixes the limit.

* fix(s3): force upload part tasks to be serial (#2447)

* feat(aws-android-sdk-core): update models to latest (#2445)

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* release: AWS SDK for Android 2.22.7 (#2451)

* release: AWS SDK for Android 2.23.0

* Update CHANGELOG.md

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* Update CHANGELOG.md

* Update gradle.properties

* Update CHANGELOG.md

* Update CHANGELOG.md

Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>
Co-authored-by: Chang Xu <42978935+changxu0306@users.noreply.github.com>
Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* "feat(aws-android-sdk-cognitoidentityprovider): support custom endpoint" (#2455)

* fix(pinpoint): add campaign attributes to push events (#2458)

* release: AWS SDK for Android 2.23.0 (#2459)

* release: AWS SDK for Android 2.22.8

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update gradle.properties

Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>
Co-authored-by: Chang Xu <42978935+changxu0306@users.noreply.github.com>

* feat(aws-android-sdk-sns): update models to latest (#2461)

* feat(aws-android-sdk-cognitoidentityprovider): update models to latest (#2456)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* chore(build): set region in circleci script (#2467)

* fix: launch hosted-ui sign-out using custom tabs manager (#2472)

* feat(mobile-client): hosted-ui auth response handler is now built into redirect activity (#2473)

* feat(mobile-client): auth response handler is now built into redirect activity

* add javadocs for redirect activities

* add signout latch conditionally

* add no history flag to auth signout flow

* feat(aws-android-sdk-connect): update models to latest (#2469)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-transcribe): update models to latest (#2476)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-rekognition): update models to latest (#2487)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-iot): update models to latest (#2490)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>
Co-authored-by: Rafael Juliano <rjjulian@amazon.com>

* feat(aws-android-sdk-location): update models to latest (#2494)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-sns): update models to latest (#2496)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-polly): update models to latest (#2497)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* chore(sts): add support for regionalizing sts client (#2493)

* feat(sts): add support for regionalizing sts client

* feat(aws-android-sdk-mobile-client): adds signature with user attributes in confirmSignIn (#2492)

* feat(aws-android-sdk-mobile-client): adds signature with user attributes in confirmSignIn

* code review suggestion

Co-authored-by: Noyes <dnnoyes@f8ffc25e9e15.ant.amazon.com>

* release: AWS SDK for Android 2.24.0 (#2500)

* release: AWS SDK for Android 2.24.0

* Reword the changelog

* include instruction for applying fix

Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>
Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* fix(aws-android-sdk-lex): prioritize custom lex signer for all regions (#2506)

* fix(aws-android-sdk-lex): prioritize custom lex signer for all regions

* add tests

* fix(mobileclient): Honor auth flow setting from config (#2499)

* fix(mobileclient): Honor auth flow setting from config

* PR feedback

* fix(aws-android-sdk-polly): use correct SignerConfig in all regions (#2505)

Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>

* feat(aws-android-sdk-cognitoidentityprovider): update models to latest (#2510)

* release: AWS SDK for Android 2.25.0 (#2512)

Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>

* chore(docs): releases not pushed to S3 anymore (#2514)

* fix(aws-android-sdk-s3): implement retry mechanism for upload part (#2504)

* implement retry mechanism for upload part

* reduce backoff time and max attempts

* lgtm warning

* feat(aws-android-sdk-connect): update models to latest (#2516)

* feat(aws-android-sdk-kms): update models to latest (#2518)

* release: AWS SDK for Android 2.26.0 (#2525)

Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>

* feat(aws-android-sdk-connect): update models to latest (#2526)

Co-authored-by: Abhash Kumar Singh <abhashs@amazon.com>
Co-authored-by: Abhash Kumar Singh <thisisabhash@gmail.com>
Co-authored-by: Jameson Williams <jhwill@amazon.com>
Co-authored-by: AWS Mobile SDK Team <46607340+awsmobilesdk@users.noreply.github.com>
Co-authored-by: Richard McClellan <ricmccle@amazon.com>
Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>
Co-authored-by: Rafael Juliano <rjjulian@amazon.com>
Co-authored-by: Daniel Rochetti <daniel.rochetti@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>
Co-authored-by: Chang Xu <42978935+changxu0306@users.noreply.github.com>
Co-authored-by: Dustin Noyes <dustin.noyes.dev@gmail.com>
Co-authored-by: Noyes <dnnoyes@f8ffc25e9e15.ant.amazon.com>
Co-authored-by: tllauda <85560392+tllauda@users.noreply.github.com>

* Update UserPoolClientType.java

remove duplicate vars.

* delete code for removed API

* fixes:
 - use access token to check claim
 - check for origin_jti claim
 - clientSecret is optional

* Update aws-android-sdk-cognitoidentityprovider/src/main/java/com/amazonaws/mobileconnectors/cognitoidentityprovider/CognitoUser.java

Co-authored-by: Richard McClellan <ricmccle@amazon.com>

* swallow exceptions

* update test mock classes with latest model changes

* add unit tests

* return revoketoken response result

Co-authored-by: Divyesh Chitroda <div5yesh@gmail.com>
Co-authored-by: Abhash Kumar Singh <abhashs@amazon.com>
Co-authored-by: Abhash Kumar Singh <thisisabhash@gmail.com>
Co-authored-by: AWS Mobile SDK Team <46607340+awsmobilesdk@users.noreply.github.com>
Co-authored-by: Richard McClellan <ricmccle@amazon.com>
Co-authored-by: Raphael Kim <52714340+raphkim@users.noreply.github.com>
Co-authored-by: Rafael Juliano <rjjulian@amazon.com>
Co-authored-by: Daniel Rochetti <daniel.rochetti@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: awsmobilesdk-dev+ghops <awsmobilesdk-dev+ghops@amazon.com>
Co-authored-by: Chang Xu <42978935+changxu0306@users.noreply.github.com>
Co-authored-by: Dustin Noyes <dustin.noyes.dev@gmail.com>
Co-authored-by: Noyes <dnnoyes@f8ffc25e9e15.ant.amazon.com>
Co-authored-by: tllauda <85560392+tllauda@users.noreply.github.com>
Co-authored-by: poojamat <mathpooj@amazon.com>
@arunj-kp
Copy link
Contributor

@raphkim I don't think it is a good idea to disable multithreading for multipart uploads. Multithreads give high performance boost while uploading files greater than 20MB for the users on high bandwidth network (like >100Mbps). So, we need to investigate and fix the reason for upload failure on multithreads rather than disabling it.
What is the behavior in the iOS SDK? does it support multithreading for multipart uploads?

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

3 participants