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

perf: fulfill promises to upload files to S3 concurrently #4575

Conversation

nchaloult
Copy link
Contributor

Issue #, if available:
#4158

Description of changes:
When uploading multiple files to S3 after running $ amplify publish, those uploads are handled one after the other when they could be handled concurrently. Rather than fulfilling promises to upload each file sequentially, this PR proposes that we let the node runtime handle fulfilling those promises concurrently with Promise.all().

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@nchaloult
Copy link
Contributor Author

It looks like the original member of the open source community who created #4158 opened a PR of their own to resolve this issue: #4508. After taking a look at it, I'm not sure if their work is correct. I'd like to hear what everyone else thinks.

uploadFileTasks is an array of anonymous functions that return promises, because sequential() expects an array of functions that return promises as its argument. Promise.all(), however, expects an iterable of promises as its argument, so the logic that builds the items in the uploadFileTasks array needs to be changed accordingly if sequential() is to be swapped out for Promise.all().

I confirmed through local testing with amplify-dev that if sequential() call is replaced with Promise.all(), but no changes are made to the way that the uploadFileTasks array is constructed, no files are ever uploaded to an S3 bucket.

@codecov
Copy link

codecov bot commented Jun 16, 2020

Codecov Report

Merging #4575 into master will decrease coverage by 0.52%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4575      +/-   ##
==========================================
- Coverage   61.57%   61.04%   -0.53%     
==========================================
  Files         333      333              
  Lines       14550    14704     +154     
  Branches     2764     2782      +18     
==========================================
+ Hits         8959     8976      +17     
- Misses       5164     5299     +135     
- Partials      427      429       +2     
Impacted Files Coverage Δ
...s/awscloudformation/utils/trigger-file-uploader.js 17.94% <0.00%> (ø)
...ejs-function-runtime-provider/src/utils/execute.ts 75.47% <50.00%> (-0.46%) ⬇️
...aphql-auth-transformer/src/ModelAuthTransformer.ts 87.96% <50.00%> (+0.10%) ⬆️
...sting/lib/S3AndCloudFront/helpers/file-uploader.js 77.55% <100.00%> (-0.89%) ⬇️
...-model-plugin/src/visitors/appsync-java-visitor.ts 90.75% <100.00%> (+0.16%) ⬆️
packages/graphql-transformer-core/src/index.ts 96.77% <100.00%> (-0.20%) ⬇️
...ackages/graphql-transformer-core/src/validation.ts 97.67% <100.00%> (ø)
...kages/amplify-util-mock/src/utils/lambda/invoke.ts
...kages/amplify-util-mock/src/__e2e__/utils/index.ts
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c04c28...37333cf. Read the comment docs.

@nchaloult nchaloult added this to Not started ⏳ in Fellowship Contribution Board Jun 16, 2020
@nchaloult nchaloult moved this from Not started ⏳ to Pending Review 👀 in Fellowship Contribution Board Jun 16, 2020
@GeorgeBellTMH
Copy link

I am fine with closing my PR...it wasn't working, and I have had other stuff on my plate...

@ammarkarachi ammarkarachi self-assigned this Jun 22, 2020
@ammarkarachi ammarkarachi self-requested a review June 22, 2020 16:19
@nchaloult nchaloult force-pushed the 4158-publish-to-s3-concurrently branch from 55184fe to 37333cf Compare June 22, 2020 17:15
@ammarkarachi ammarkarachi merged commit 96d1914 into aws-amplify:master Jul 10, 2020
Fellowship Contribution Board automation moved this from Pending Review 👀 to Complete 🙌 Jul 10, 2020
UnleashedMind pushed a commit to UnleashedMind/amplify-cli that referenced this pull request Jul 10, 2020
ammarkarachi pushed a commit that referenced this pull request Jul 10, 2020
* feat(amplify-provider-awscloudformation): add region ca-central-1

* fix test

* Revert "Add region ca-central-1 (#4796)"

This reverts commit c8b6bd8.

* Revert "feat: add check for extra command line args provided with amplify delete (#4576)"

This reverts commit 82d1093.

* Revert "perf: fulfill promises to upload files to S3 concurrently (#4575)"

This reverts commit 96d1914.

* Revert "fix(cli): remove unnecessary stack trace log when adding services (#4610)"

This reverts commit 5bee574.

* Revert "fix test"

This reverts commit 63c3c78.

Co-authored-by: UnleashedMind <zhoweimi@186590ce137f.ant.amazon.com>
nikhname pushed a commit to nikhname/amplify-cli that referenced this pull request Jul 27, 2020
nikhname pushed a commit to nikhname/amplify-cli that referenced this pull request Jul 27, 2020
* feat(amplify-provider-awscloudformation): add region ca-central-1

* fix test

* Revert "Add region ca-central-1 (aws-amplify#4796)"

This reverts commit af29f35.

* Revert "feat: add check for extra command line args provided with amplify delete (aws-amplify#4576)"

This reverts commit 48b27b7.

* Revert "perf: fulfill promises to upload files to S3 concurrently (aws-amplify#4575)"

This reverts commit 9fbee8a.

* Revert "fix(cli): remove unnecessary stack trace log when adding services (aws-amplify#4610)"

This reverts commit 56efb32.

* Revert "fix test"

This reverts commit 50a8073fbbdb56931191912938fb50f542197832.

Co-authored-by: UnleashedMind <zhoweimi@186590ce137f.ant.amazon.com>
sebastiancrossa pushed a commit to MLH-Fellowship/amplify-cli that referenced this pull request Aug 4, 2020
sebastiancrossa pushed a commit to MLH-Fellowship/amplify-cli that referenced this pull request Aug 4, 2020
* feat(amplify-provider-awscloudformation): add region ca-central-1

* fix test

* Revert "Add region ca-central-1 (aws-amplify#4796)"

This reverts commit af29f35.

* Revert "feat: add check for extra command line args provided with amplify delete (aws-amplify#4576)"

This reverts commit 48b27b7.

* Revert "perf: fulfill promises to upload files to S3 concurrently (aws-amplify#4575)"

This reverts commit 9fbee8a.

* Revert "fix(cli): remove unnecessary stack trace log when adding services (aws-amplify#4610)"

This reverts commit 56efb32.

* Revert "fix test"

This reverts commit 50a8073fbbdb56931191912938fb50f542197832.

Co-authored-by: UnleashedMind <zhoweimi@186590ce137f.ant.amazon.com>
sebastiancrossa pushed a commit to MLH-Fellowship/amplify-cli that referenced this pull request Aug 25, 2020
sebastiancrossa pushed a commit to MLH-Fellowship/amplify-cli that referenced this pull request Aug 25, 2020
* feat(amplify-provider-awscloudformation): add region ca-central-1

* fix test

* Revert "Add region ca-central-1 (aws-amplify#4796)"

This reverts commit af29f35.

* Revert "feat: add check for extra command line args provided with amplify delete (aws-amplify#4576)"

This reverts commit 48b27b7.

* Revert "perf: fulfill promises to upload files to S3 concurrently (aws-amplify#4575)"

This reverts commit 9fbee8a.

* Revert "fix(cli): remove unnecessary stack trace log when adding services (aws-amplify#4610)"

This reverts commit 56efb32.

* Revert "fix test"

This reverts commit 50a8073fbbdb56931191912938fb50f542197832.

Co-authored-by: UnleashedMind <zhoweimi@186590ce137f.ant.amazon.com>
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants