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

feat(storage): adding lib/storage upload #1547

Merged
merged 20 commits into from
Oct 19, 2020

Conversation

alexforsyth
Copy link
Contributor

@alexforsyth alexforsyth commented Sep 30, 2020

Issue #, if available:

Description of changes:

This feature is called multipart upload in JS SDK V2. This is how customers might use it.

import { S3 } from "aws-sdk";

(async () => {
    const payload = fs.createReadStream("./big.file")
    const putRequest: S3.Types.PutObjectRequest = {
        Bucket: "aws-sdk-js-mock-files", 
        Key: 'data_key', 
        Body: payload
    }

    const options = {
        params: putRequest
    };

    let upload = new S3.ManagedUpload(options);
    upload.on("httpUploadProgress", (progress) => {
        console.log(progress);
    });
    await upload.send();
})();

Proposed Customer code:

This is how we propose to do it in v3. "@aws-operations/storage" contains many high level operations relating to storage, one of which is upload.

import { Upload } from "../src/index";
import { S3 } from "@aws-sdk/client-s3";

const fs = require("fs");
const fileStream = fs.createReadStream("./big.file");

(async () => {
  const target = {
    Bucket: "aws-sdk-js-mock-files",
    Key: "data_key",
    Body: fileStream,
  };

  try {
    const upload = new Upload({
      params: target,
      client: new S3({}),
      queueSize: 3,
    });

    upload.on("httpUploadProgress", (progress) => {
      console.log(progress);
    });

    await upload.done();
  } catch (e) {
    console.log(e);
  }
})();


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

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2020

Codecov Report

Merging #1547 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1547      +/-   ##
==========================================
- Coverage   79.80%   79.71%   -0.09%     
==========================================
  Files         298      302       +4     
  Lines       11502    11678     +176     
  Branches     2475     2489      +14     
==========================================
+ Hits         9179     9309     +130     
- Misses       2323     2369      +46     
Impacted Files Coverage Δ
...l_tests/aws-json/commands/EmptyOperationCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
...ts/aws-query/commands/NoInputAndNoOutputCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
.../aws-restxml/commands/NoInputAndNoOutputCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
...aws-restjson/commands/NoInputAndNoOutputCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
protocol_tests/aws-ec2/commands/XmlBlobsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
protocol_tests/aws-ec2/commands/XmlEnumsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
protocol_tests/aws-ec2/commands/XmlListsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
...rotocol_tests/aws-query/commands/XmlMapsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
...otocol_tests/aws-query/commands/XmlBlobsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
...otocol_tests/aws-query/commands/XmlEnumsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
... and 128 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 ddcf0c3...ac3b0e5. Read the comment docs.

@alexforsyth alexforsyth force-pushed the experimental/storage-upload branch 2 times, most recently from 3f1de3f to ac3b0e5 Compare October 2, 2020 19:55
@AllanZhengYP
Copy link
Contributor

You need to add experimental folder to the workspace config to make Lerna recognize this package:

"packages": [
"packages/*",
"clients/*",
"protocol_tests/*"
],

experimental/storage/src/Storage.ts Outdated Show resolved Hide resolved
experimental/storage/src/upload/types.ts Outdated Show resolved Hide resolved
experimental/storage/src/upload/types.ts Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Oct 15, 2020

Codecov Report

Merging #1547 into master will decrease coverage by 0.33%.
The diff coverage is 61.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1547      +/-   ##
==========================================
- Coverage   79.92%   79.58%   -0.34%     
==========================================
  Files         313      324      +11     
  Lines       11839    12056     +217     
  Branches     2504     2547      +43     
==========================================
+ Hits         9462     9595     +133     
- Misses       2377     2461      +84     
Impacted Files Coverage Δ
packages/util-create-request/src/index.ts 100.00% <ø> (ø)
lib/storage/src/utils/bytelength.ts 14.28% <14.28%> (ø)
lib/storage/src/upload/service-clients/Uploader.ts 26.19% <26.19%> (ø)
...b/storage/src/upload/service-clients/S3Uploader.ts 50.00% <50.00%> (ø)
...age/src/upload/service-clients/S3ClientUploader.ts 52.63% <52.63%> (ø)
lib/storage/src/upload/Upload.ts 55.26% <55.26%> (ø)
lib/storage/src/data-chunk/yield-chunk.ts 89.47% <89.47%> (ø)
lib/storage/src/data-chunk/buffer-helper.ts 91.66% <91.66%> (ø)
lib/storage/src/data-chunk/readable-helper.ts 92.30% <92.30%> (ø)
...b/storage/src/data-chunk/readable-stream-helper.ts 96.15% <96.15%> (ø)
... and 14 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 16d75c5...1c92a4f. Read the comment docs.

@alexforsyth alexforsyth changed the title WIP: Experimental/storage upload feat(storage): adding lib/storage upload Oct 15, 2020
lib/storage/README.md Outdated Show resolved Hide resolved
lib/storage/README.md Outdated Show resolved Hide resolved
lib/storage/src/data-chunk/readable-helper.ts Show resolved Hide resolved
lib/storage/src/data-chunk/buffer-helper.ts Show resolved Hide resolved
lib/storage/package.json Show resolved Hide resolved
lib/storage/src/upload/defaults.ts Outdated Show resolved Hide resolved
lib/storage/src/upload/service-clients/Uploader.ts Outdated Show resolved Hide resolved
lib/storage/src/upload/Upload.ts Outdated Show resolved Hide resolved
lib/storage/src/upload/service-clients/Uploader.ts Outdated Show resolved Hide resolved
lib/storage/src/upload/service-clients/Uploader.ts Outdated Show resolved Hide resolved
@AllanZhengYP
Copy link
Contributor

Browser test is necessary multipart upload. Can you create a Karma smoke test on browsers? It only needs to cover the code path that's executed in browsers(ReadableStream, etc.)? It's also important to verify the code works with bundlers? See here for an example using Webpack, or here for an example using Browserify.

Copy link
Contributor

@AllanZhengYP AllanZhengYP 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 to me. But the leavePartsOnError config needs to negate.

lib/storage/README.md Outdated Show resolved Hide resolved
lib/storage/src/upload/Upload.ts Outdated Show resolved Hide resolved
lib/storage/src/upload/Upload.ts Outdated Show resolved Hide resolved
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

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

Successfully merging this pull request may close these issues.

None yet

4 participants