feat: MPU for transfer manager #2192

ddelgrosso1 · 2023-05-04T19:55:24Z

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

danielduhh

Very exciting! Just had some initial thoughts/questions...

danielduhh · 2023-05-04T22:32:58Z

src/transfer-manager.ts

+      });
+      for await (const curChunk of readStream) {
+        if (promises.length >= maxQueueSize) {
+          await Promise.all(promises);


what does this do? Execute all the parts? Should this get returned to the caller?

This is similar to the changes I made for deleteFiles. p-limit only constrains the number of running promises, not the number of promises being held in memory. I added a queue here such that if a user is uploading a large file we aren't attempting to hold all those chunks in memory at once. By default we will only hold the same number of chunks in memory as the number of executing promises.

Makes sense. Maybe add a comment to make it more clear?

Added a comment to explain the logic.

danielduhh · 2023-05-04T22:34:41Z

src/transfer-manager.ts

+        );
+      }
+      await Promise.all(promises);
+      await mpuHelper.completeUpload();


What's the return statement here?

The return is of type Promise<void> in the success case there is no return value to the user. If something goes wrong we throw an exception with the error, uploadId, and mapping of completed parts that were uploaded. This allows the user to call again supplying the uploadId and parts so that we may resume from where the failure occurred.

Ah I see. Is there no value in returning the response from complete upload?

I will change it to return the full response just to be inline with the raw API.

Now returns Promise<GaxiosResponse | undefined>. The GaxiosResponse is the full API response.

danielduhh · 2023-05-04T22:35:08Z

src/transfer-manager.ts

+ * @private
+ * @experimental
+ */
+class XMLMultiPartUploadHelper implements MultiPartUploadHelper {


should this go in a separate file?

I can move this to a separate file the only problem is I will have to export it which means a user can potentially come along and import it and use it directly. I was attempting to avoid that situation. Depends what people think and whether or not we are ok with saying "don't use this directly".

danielduhh · 2023-05-04T22:36:29Z

internal-tooling/performTransferManagerTest.ts

 *
 * @returns {Promise<TestResult>} A promise that resolves containing information about the test results.
 */
-async function performDownloadFileInChunksTest(): Promise<TestResult> {
+async function performChunkUploadDownloadTest(): Promise<TestResult[]> {


Whats the reasoning for combining these? Simplicity?

Workload 8 is supposed to be upload and download test so I combined them.

danielduhh · 2023-05-04T22:38:26Z

src/transfer-manager.ts

 import {CRC32C} from './crc32c';
+import {GoogleAuth} from 'google-auth-library';
+import {XMLParser, XMLBuilder} from 'fast-xml-parser';


Add to package JSON? It would be interesting to see the level of effort for adding this to one of our shared libs (gaxios?).

I added it: /~https://github.com/googleapis/nodejs-storage/pull/2192/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R62. I'm sure we could write something ourselves. I figured since this was a one off usage of the XML API it probably wasn't worth doing ourselves.

danielduhh

looks good. One question on returning the response of complete upload

danielduhh · 2023-05-05T19:25:02Z

src/transfer-manager.ts

+      });
+      for await (const curChunk of readStream) {
+        if (promises.length >= maxQueueSize) {
+          await Promise.all(promises);


Makes sense. Maybe add a comment to make it more clear?

danielduhh · 2023-05-05T19:30:16Z

src/transfer-manager.ts

+        );
+      }
+      await Promise.all(promises);
+      await mpuHelper.completeUpload();


Ah I see. Is there no value in returning the response from complete upload?

ddelgrosso1 added 2 commits May 4, 2023 19:53

feat: MPU for transfer manager

6caf073

add tests for MPU upload

7fc246d

ddelgrosso1 requested review from a team as code owners May 4, 2023 19:55

product-auto-label bot added size: l Pull request size is large. api: storage Issues related to the googleapis/nodejs-storage API. labels May 4, 2023

naming fix in tests

a9bd687

ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 4, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 4, 2023

danielduhh reviewed May 4, 2023

View reviewed changes

danielduhh approved these changes May 5, 2023

View reviewed changes

ddelgrosso1 added 2 commits May 5, 2023 20:18

return full response from completeUpload

fc80642

add comment about queueing mechanism

b0b37d7

ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 5, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 5, 2023

ddelgrosso1 requested review from andrewsg and tritone May 5, 2023 21:36

fix typo

46b4f7e

ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 5, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 5, 2023

add md5 validation to uploadFileInChunks

0a88267

ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 16, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 16, 2023

ddelgrosso1 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 24, 2023

ddelgrosso1 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 10, 2023

ddelgrosso1 merged commit ae83421 into googleapis:main Jul 12, 2023

ddelgrosso1 deleted the tm-chunk-up branch July 12, 2023 15:10

release-please bot mentioned this pull request Jul 12, 2023

chore(main): release 6.12.0 #2233

Merged

punehim mentioned this pull request Sep 6, 2024

[Snyk] Upgrade @google-cloud/storage from 6.11.0 to 6.12.0 punehim/GCP-cloud-foundation-fabric#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: MPU for transfer manager #2192

feat: MPU for transfer manager #2192

ddelgrosso1 commented May 4, 2023

danielduhh left a comment

danielduhh May 4, 2023

ddelgrosso1 May 5, 2023

danielduhh May 5, 2023

ddelgrosso1 May 5, 2023

danielduhh May 4, 2023

ddelgrosso1 May 5, 2023

danielduhh May 5, 2023

ddelgrosso1 May 5, 2023 •

edited

Loading

ddelgrosso1 May 5, 2023

danielduhh May 4, 2023

ddelgrosso1 May 5, 2023

danielduhh May 4, 2023

ddelgrosso1 May 5, 2023

danielduhh May 4, 2023

ddelgrosso1 May 5, 2023

danielduhh left a comment

danielduhh May 5, 2023

danielduhh May 5, 2023

feat: MPU for transfer manager #2192

feat: MPU for transfer manager #2192

Conversation

ddelgrosso1 commented May 4, 2023

danielduhh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddelgrosso1 May 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielduhh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddelgrosso1 May 5, 2023 •

edited

Loading