feat: implement parallel operations by ddelgrosso1 · Pull Request #2067 · googleapis/nodejs-storage

ddelgrosso1 · 2022-09-15T20:57:39Z

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 #1868 🦕
Fixes #2051

ddelgrosso1 · 2022-09-15T20:58:12Z

Opening as a draft PR while I add in JSDoc comments so that I can start gathering feedback / initial thoughts.

ddelgrosso1 · 2022-09-16T14:50:37Z

Repointing this at a new branch named transfer-manager. My plan is to release this as a "preview" from that branch. Since this is a new feature, keeping it in sync with main should be relatively headache free until such a point that we deem the feature stable.

danielduhh

Just a couple passthrough comments/questions :)

* fix: more work on transfer manager perf metrics * fix unit tests for tm

* refactor: refactor constants * bug fixes * bug fixes * bug fixes

shaffeeullah

Few comments, but generally looks good. Thanks, Denis!!

shaffeeullah · 2022-11-29T20:38:05Z

+   * ```
+   * @experimental
+   */
+  async downloadManyFiles(


do other lanauges have this as a list of objects? or is a list of strings also an option?

Python does it as a list of names. If desired we could move to that as well. Thoughts?

The two use cases are

download "directory"

download list of files

Not sure which is more common (question for @domZippilli) but it would be great to support both (or show users how to do both)

I've modified the signature to now accept an array of files, an array of strings, or a single string. If a single string is provided it will be treated as a GCS prefix to call getFiles. Utilizing this prefix will allow for an entire folder structure to be downloaded.

That's probably the right idea. It sounds like an idiomatic variation on what I wrote before I saw @ddelgrosso1's reply.

A directory is probably the most common set of files people upload. But of course you'd want to think about this as a list of files to be extensible. To me, the logical progression is something like "I want to upload a directory" -> "I want to form a glob of file names" -> "I want to upload a list of files". So solving the last problem lets you frame the others as variations on it. For example, make a convenience method that accepts a directory name, and just constructs the list of files to pipe into the actual implementation.

shaffeeullah · 2022-11-29T20:42:49Z

+      assert.strictEqual(count, paths.length);
+    });
+
+    it('sets ifGenerationMatch to 0 if skipIfExists is set', async () => {


is there a test for skipIfExists set to true, the file exists, and it errors? We should verify that it doesnt stop the execution of the code and just moves onto the next file.

I'm not sure how to best handle this as during a unit test since we aren't calling the actual service.

danielduhh · 2022-11-30T00:26:42Z

+   * ```
+   * @experimental
+   */
+  async downloadManyFiles(


The two use cases are

download "directory"

download list of files

Not sure which is more common (question for @domZippilli) but it would be great to support both (or show users how to do both)

… folder name

snippet-bot · 2022-12-01T20:47:45Z

Here is the summary of changes.

You are about to add 3 region tags.

samples/downloadFileInChunksWithTransferManager.js:32, tag storage_download_many_files_transfer_manager
samples/downloadManyFilesWithTransferManager.js:28, tag storage_download_many_files_transfer_manager
samples/uploadManyFilesWithTransferManager.js:28, tag storage_upload_many_files_transfer_manager

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

danielbankhead

Looks good, a few thoughts/comments. Gonna play with it some more tomorrow.

danielbankhead · 2022-12-02T00:43:21Z

+ * limitations under the License.
+ */
+
+// eslint-disable-next-line node/no-unsupported-features/node-builtins


Is this comment still needed?

Yes, this file requires worker_theads and not all features were available in the initial release of v12. If I remove this the CI pipeline starts to complain if I remember correctly.

danielbankhead · 2022-12-02T02:23:53Z

+   * @experimental
+   */
+  async uploadManyFiles(
+    filePaths: string[],


[Optional] We have an opportunity to get fancy here to really emphasize throughput; what if we allowed iterators and async iterators?

Use cases:

walking large directory you want to walk asynchronously (reduces memory usage)

situations where one may parse a list of files in chunks and want to chain operations together (such as an HTTP server receiving incoming data)

We can probably wait until customers ask for this functionality though.

It is a good idea, maybe after the initial release we can circle up. I would definitely be interested in your thoughts on optimizations here.

danielbankhead · 2022-12-02T02:31:02Z

+  async downloadFileInChunks(
+    fileOrName: File | string,
+    options: DownloadFileInChunksOptions = {}
+  ): Promise<void | DownloadResponse> {


What are your thoughts on data validation with this method? Since file.download({start, end}) skips validation, we may want to add support or mention it in the description.

Good question... I will definitely add a callout in the comments that this will skip validation. Do we currently have a means of validating a file after it has been downloaded? I know you did a bunch of work in this area but I thought it only worked with streamed input?

I believe it's technically possible to CRC32C chunks and then "sum" them into the final checksum, such that you could do this kind of chunked operation and still checksum without an additional byte scan. If you need MD5 or can't do the previous algorithm, I think validation would require a full read of the completed file.

Do we currently have a means of validating a file after it has been downloaded?

Yep (although we may be able to do this as we're receiving each chunk):

nodejs-storage/src/crc32c.ts

Line 257 in a61e619

static async fromFile(file: PathLike) {

I know you did a bunch of work in this area but I thought it only worked with streamed input?

It's possible to use CRC32C with/out streams (HashStreamValidator is the class that wraps CRC32C for streams):

nodejs-storage/src/crc32c.ts

Line 134 in a61e619

class CRC32C implements CRC32CValidator {

nodejs-storage/src/hash-stream-validator.ts

Lines 30 to 34 in a61e619

class HashStreamValidator extends Transform {

readonly crc32cEnabled: boolean;

readonly md5Enabled: boolean;

#crc32cHash?: CRC32CValidator = undefined;

Are we comfortable with only allowing CRC32C. In my mind something is better than nothing... I can take a look at implementing it if we feel strongly this should be included.

danielbankhead · 2022-12-02T02:31:32Z

+    const fileToWrite = await fsp.open(filePath, 'w+');
+    while (start < size) {
+      const chunkStart = start;
+      let chunkEnd = start + chunkSize - 1;
+      chunkEnd = chunkEnd > size ? size : chunkEnd;
+      promises.push(
+        limit(() =>
+          file.download({start: chunkStart, end: chunkEnd}).then(resp => {
+            return fileToWrite.write(resp[0], 0, resp[0].length, chunkStart);
+          })
+        )
+      );
+
+      start += chunkSize;
+    }


What's stopping the chunks from being written out of order?

If I did this correctly the position in the file of where to write this chunk is handled by the chunkStart parameter. Basically this call should write the incoming chunk with no offset in the chunk buffer to the file offset specified by chunkStart. Was your understanding different?

I have a slightly different understanding here (I could be wrong), but would it be possible for subsequent chunks to download before proceeding chunks and write to the file first?

Say we have a 65MB file and a 64MB chunk size (making 2 chunks to download). Since the 2nd chunk is 1MB and would download first, wouldn't it write to the file before the first chunk?

It shouldn't matter if they are written in order. This implementation is leaning on a filesystem supporting sparse files. So, imagine you wrote these back-to-front; the first chunk will seek to its chunkStart near the end of the file, and the OS will logically pad that first byte with 0s up to the seek point.

^that's the missing piece for me, thanks Dom!

As Dom outlined this was the behavior I was intending. It shouldn't matter which chunk downloads first they should get written to their correct place in the resultant file. Ultimately if all succeed, the file should be correctly "reassembled".

ddelgrosso1 · 2022-12-05T16:04:41Z

Since this is getting large and most feedback has been addressed going to go ahead and merge this into the staging branch. We can re-review before the final merge to main.

* feat: implement parallel operations * add more parallel operations * add header to test file * update import of fs/promises * fix pathing on windows, fix mocking of fs promises * add jsdoc headers to class and uploadMulti * add jsdoc comments to remaining functions * update comment wording * add experimental jsdoc tags * feat: add directory generator to performance test framework * clarify variable names and comments * capitalization * wip: transfer manager performance tests * feat: merged in application performance tests (googleapis#2100) * fix: fixed many bugs (googleapis#2102) * fix: cleaning up bugs * fix: fixed many bugs * fix: more work on transfer manager perf metrics (googleapis#2103) * fix: more work on transfer manager perf metrics * fix unit tests for tm * fix: performance test refactoring, comments (googleapis#2104) * refactor: refactor constants (googleapis#2105) * refactor: refactor constants * bug fixes * bug fixes * bug fixes * linter fixes, download to disk for performance test * rename transfer manager functions * remove callbacks from transfer manager * add more experimental tags, update comments * change signature of downloadManyFiles to accept array of strings or a folder name * linter fix * add transfer manager samples and samples tests Co-authored-by: Sameena Shaffeeullah <shaffeeullah@google.com>

* feat: implement parallel operations * add more parallel operations * add header to test file * update import of fs/promises * fix pathing on windows, fix mocking of fs promises * add jsdoc headers to class and uploadMulti * add jsdoc comments to remaining functions * update comment wording * add experimental jsdoc tags * feat: add directory generator to performance test framework * clarify variable names and comments * capitalization * wip: transfer manager performance tests * feat: merged in application performance tests (#2100) * fix: fixed many bugs (#2102) * fix: cleaning up bugs * fix: fixed many bugs * fix: more work on transfer manager perf metrics (#2103) * fix: more work on transfer manager perf metrics * fix unit tests for tm * fix: performance test refactoring, comments (#2104) * refactor: refactor constants (#2105) * refactor: refactor constants * bug fixes * bug fixes * bug fixes * linter fixes, download to disk for performance test * rename transfer manager functions * remove callbacks from transfer manager * add more experimental tags, update comments * change signature of downloadManyFiles to accept array of strings or a folder name * linter fix * add transfer manager samples and samples tests Co-authored-by: Sameena Shaffeeullah <shaffeeullah@google.com>

* feat: implement parallel operations (#2067) * feat: implement parallel operations * add more parallel operations * add header to test file * update import of fs/promises * fix pathing on windows, fix mocking of fs promises * add jsdoc headers to class and uploadMulti * add jsdoc comments to remaining functions * update comment wording * add experimental jsdoc tags * feat: add directory generator to performance test framework * clarify variable names and comments * capitalization * wip: transfer manager performance tests * feat: merged in application performance tests (#2100) * fix: fixed many bugs (#2102) * fix: cleaning up bugs * fix: fixed many bugs * fix: more work on transfer manager perf metrics (#2103) * fix: more work on transfer manager perf metrics * fix unit tests for tm * fix: performance test refactoring, comments (#2104) * refactor: refactor constants (#2105) * refactor: refactor constants * bug fixes * bug fixes * bug fixes * linter fixes, download to disk for performance test * rename transfer manager functions * remove callbacks from transfer manager * add more experimental tags, update comments * change signature of downloadManyFiles to accept array of strings or a folder name * linter fix * add transfer manager samples and samples tests Co-authored-by: Sameena Shaffeeullah <shaffeeullah@google.com> * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md * feat: add crc32c validation option to chunked download, cleanup naming (#2110) * feat: add crc32c validation option to chunked download, cleanup naming in perf tests * close file in finally block Co-authored-by: Sameena Shaffeeullah <shaffeeullah@google.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

product-auto-label Bot added size: l Pull request size is large. api: storage Issues related to the googleapis/nodejs-storage API. labels Sep 15, 2022

ddelgrosso1 changed the base branch from main to transfer-manager September 16, 2022 14:46

danielduhh reviewed Sep 21, 2022

View reviewed changes

Comment thread src/transfer-manager.ts

Comment thread src/transfer-manager.ts Outdated

Comment thread src/transfer-manager.ts

ddelgrosso1 force-pushed the parallel-operations branch from 3e26dd8 to ad6aadd Compare September 30, 2022 17:41

product-auto-label Bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Sep 30, 2022

ddelgrosso1 changed the base branch from transfer-manager to main September 30, 2022 17:50

ddelgrosso1 changed the base branch from main to transfer-manager September 30, 2022 17:51

shaffeeullah reviewed Oct 13, 2022

View reviewed changes

Comment thread src/transfer-manager.ts

Comment thread src/transfer-manager.ts Outdated

Comment thread src/transfer-manager.ts

Comment thread src/transfer-manager.ts Outdated

Comment thread test/transfer-manager.ts

ddelgrosso1 added 8 commits October 24, 2022 14:58

feat: implement parallel operations

8717527

add more parallel operations

22097df

add header to test file

9a59f1f

update import of fs/promises

d2b241f

fix pathing on windows, fix mocking of fs promises

30b45d3

add jsdoc headers to class and uploadMulti

16a5f05

add jsdoc comments to remaining functions

5a94aa7

update comment wording

1cc8bae

ddelgrosso1 force-pushed the parallel-operations branch from ad6aadd to 1cc8bae Compare October 24, 2022 15:25

product-auto-label Bot added size: l Pull request size is large. and removed size: xl Pull request size is extra large. labels Oct 24, 2022

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

ddelgrosso1 marked this pull request as ready for review October 24, 2022 15:26

ddelgrosso1 requested review from a team October 24, 2022 15:27

ddelgrosso1 changed the base branch from transfer-manager to main October 24, 2022 15:27

ddelgrosso1 changed the base branch from main to transfer-manager October 24, 2022 15:27

ddelgrosso1 requested a review from danielbankhead October 27, 2022 16:42

ddelgrosso1 and others added 5 commits November 15, 2022 13:19

fix: more work on transfer manager perf metrics (googleapis#2103)

886dc03

* fix: more work on transfer manager perf metrics * fix unit tests for tm

fix: performance test refactoring, comments (googleapis#2104)

94b1c02

refactor: refactor constants (googleapis#2105)

9793cc6

* refactor: refactor constants * bug fixes * bug fixes * bug fixes

linter fixes, download to disk for performance test

89e8204

rename transfer manager functions

c153ab6

ddelgrosso1 force-pushed the parallel-operations branch from 81f7850 to c153ab6 Compare November 29, 2022 17:11

product-auto-label Bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Nov 29, 2022

ddelgrosso1 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 29, 2022

remove callbacks from transfer manager

4feb1c2

ddelgrosso1 requested a review from shaffeeullah November 29, 2022 18:45

shaffeeullah reviewed Nov 29, 2022

View reviewed changes

add more experimental tags, update comments

0780e50

danielduhh reviewed Nov 30, 2022

View reviewed changes

ddelgrosso1 added 3 commits November 30, 2022 16:51

change signature of downloadManyFiles to accept array of strings or a…

c47130b

… folder name

linter fix

4c2dda4

add transfer manager samples and samples tests

aad8f2b

ddelgrosso1 requested review from danielbankhead, danielduhh and shaffeeullah December 1, 2022 20:54

danielbankhead reviewed Dec 2, 2022

View reviewed changes

shaffeeullah approved these changes Dec 5, 2022

View reviewed changes

ddelgrosso1 merged commit 3c11ea2 into googleapis:transfer-manager Dec 5, 2022

release-please Bot mentioned this pull request Dec 7, 2022

chore(main): release 6.8.0 #2113

Merged

	class HashStreamValidator extends Transform {
	readonly crc32cEnabled: boolean;
	readonly md5Enabled: boolean;

	#crc32cHash?: CRC32CValidator = undefined;

Conversation

ddelgrosso1 commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddelgrosso1 commented Sep 15, 2022

Uh oh!

ddelgrosso1 commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielduhh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shaffeeullah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snippet-bot Bot commented Dec 1, 2022

Uh oh!

danielbankhead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielbankhead Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielbankhead Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielbankhead Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ddelgrosso1 commented Sep 15, 2022 •

edited

Loading

ddelgrosso1 commented Sep 16, 2022 •

edited

Loading

danielbankhead Dec 2, 2022 •

edited

Loading

danielbankhead Dec 2, 2022 •

edited

Loading

danielbankhead Dec 2, 2022 •

edited

Loading