feat!: Multiple Chunk Upload Support #486

danielbankhead · 2021-12-13T22:18:11Z

Adding support for multiple chunk uploads. Required a sizable refactor and dozens of new tests - potentially breaking change.

src/index.ts

ddelgrosso1 · 2021-12-14T22:03:23Z

Just a general comment, I took a look at the newly added code in index.ts and it makes sense to me. It looks like as of now all the existing unit and conformance tests still work. Have you verified that you do indeed see multiple HTTP requests when a chunk size is supplied, i.e. against test bench?

danielbankhead · 2021-12-14T22:16:43Z

Working on verification now, along with additional tests. Thanks for the preview review!

- https://cloud.google.com/storage/docs/performing-resumable-uploads#chunked-upload

generated-files-bot · 2021-12-21T05:28:26Z

Warning: This pull request is touching the following templated files:

README.md - README.md is managed by synthtool. However, a partials file can be used to update the README, e.g.: https://github.com/googleapis/nodejs-storage/blob/main/.readme-partials.yaml

API `writable.writableEnded` isn't available until Node 12.9.0

…load into multi-upload-by-chunk-support

package.json

ddelgrosso1 · 2021-12-28T15:44:27Z

src/index.ts

-    request: <T = any>(
-      opts: GaxiosOptions
-    ) => Promise<GaxiosResponse<T>> | GaxiosPromise<T>;
+    request: (opts: GaxiosOptions) => Promise<GaxiosResponse> | GaxiosPromise;


Was this change because it was defined as any type?

Would this potentially cause problems for folks who've provided a shim for authClient, like this:

// Use the consumer client to define storageOptions and create a GCS object. const storageOptions = { projectId: 'my_project_id', authClient: { sign: () => Promise.reject('unsupported'), getCredentials: () => Promise.reject(), request: (opts, callback) => { return oauth2Client.request(opts, callback); }, authorizeRequest: async (opts) => { opts = opts || {}; const url = opts.url || opts.uri; const headers = await oauth2Client.getRequestHeaders(url); opts.headers = Object.assign(opts.headers || {}, headers); return opts; }, }, };

Wondering if it changes the function signature if anyone has attempted doing this in TypeScript?

Good point - I reverted this change with a generic <T> (without the any)

ddelgrosso1 · 2021-12-28T16:37:52Z

Thanks for adding all of those tests.

test/test.ts

src/index.ts

tritone

Couple questions, I'm not a node expert so I'll rely on @ddelgrosso1 to give a more comprehensive review!

README.md

src/index.ts

tritone · 2021-12-28T18:06:52Z

src/index.ts

+        // be fine for the next request.
+        this.unshiftChunkBuffer(dataToPrependForResending);
+        this.numBytesWritten -= missingBytes;
+        this.lastChunkSent = Buffer.alloc(0);


Is the buffer re-allocated for each chunk?

Yes, from upstream.
Buffer.alloc(0) creates a "Fast" buffer: https://github.com/nodejs/node/blob/e4aa575b05fcf560c6b77b0fa5036328a3417442/lib/buffer.js#L359-L366

Okay-- just asking because I know in some languages repeated buffer allocs can be painful. Is there anyway to create one buffer and reuse it for each chunk? Or is that not really how it works in node?

Understandable - it doesn't really work that way in Node.js (at least for Buffer.alloc(0))

tritone

Couple questions, I'm not a node expert so I'll rely on @ddelgrosso1 to give a more comprehensive review!

src/index.ts

ddelgrosso1 · 2021-12-28T20:43:28Z

I think overall the refactor looks good. There are a few failing tests in CI.

danielbankhead · 2021-12-28T20:53:50Z

@ddelgrosso1 thanks - just fixed some tests for the #isSuccessfulResponse refactor

ddelgrosso1

Overall I don't have any major concerns, I think the code and the tests look good. I would like some other eyes on it before merging though.

bcoe

A few thoughts:

I would be tempted to call this a breaking change, since you rewrite a lot of the original implementation, along with adding the chunk upload support, I think a major bump will help protect you from any regressions, because you can make the update to @google-cloud/storage explicit, and test throughly.
When you do add this to @google-cloud/storage, I would add a few integration tests -- I think you could just generate junk data, and assert that both types of uploading work effectively.
I left a note about a potential refactor, this is non-blocking, but I'd be curious to see if it makes the logic read easier -- the intermingling of single/multi chunk behavior detracts from what to me looks like a great refactor of the library.

Overall, I really like your implementation, feels like a solid cleanup to the library, and you've done a great job of adding tests.

The only blocker for me, would be I'd like to advocate for the major bump to this library -- curious what others think.

bcoe · 2021-12-29T20:58:21Z

src/index.ts

-    request: <T = any>(
-      opts: GaxiosOptions
-    ) => Promise<GaxiosResponse<T>> | GaxiosPromise<T>;
+    request: (opts: GaxiosOptions) => Promise<GaxiosResponse> | GaxiosPromise;


Would this potentially cause problems for folks who've provided a shim for authClient, like this:

// Use the consumer client to define storageOptions and create a GCS object. const storageOptions = { projectId: 'my_project_id', authClient: { sign: () => Promise.reject('unsupported'), getCredentials: () => Promise.reject(), request: (opts, callback) => { return oauth2Client.request(opts, callback); }, authorizeRequest: async (opts) => { opts = opts || {}; const url = opts.url || opts.uri; const headers = await oauth2Client.getRequestHeaders(url); opts.headers = Object.assign(opts.headers || {}, headers); return opts; }, }, };

Wondering if it changes the function signature if anyone has attempted doing this in TypeScript?

bcoe · 2021-12-29T21:14:05Z

src/index.ts

+   *
+   * @returns if the request is ok to continue as-is
+   */
+  private async ensureUploadingSameObject() {


Is this edge case likely to happen? and what would have happened in the previous implementation of gcs-resumbable-upload?

If this is an optimization, it might be nice to do this in follow up work just to simplify this PR.

This preserves existing behavior - I don't think this is likely to happen (https://github.com/googleapis/gcs-resumable-upload/blob/main/src/index.ts#L577-L596)

bcoe · 2021-12-29T21:16:24Z

test/test.ts

+          assert.equal(data.byteLength, CHUNK_SIZE);
+        });
+
+        it('should prepare a valid request if the remaining data is less than `chunkSize`', async () => {


I appreciate how thorough these tests are 🎉

bcoe · 2021-12-29T21:25:31Z

src/index.ts

-      let cachedFirstChunk = this.get('firstChunk');
-      const firstChunk = chunk.slice(0, 16).valueOf();
+      // continue uploading next chunk
+      this.continueUploading();


It looks like the logic of continueUploading hasn't changed much. Is the upload endpoint a constant URI, and it's just Content-Range that changes to support multiple chunk uploads?

Correct - it's mainly Content-Range and supporting N requests per Upload (required a lot of decoupling throughout the class)

bcoe · 2021-12-29T21:28:40Z

src/index.ts

+      resp.status === RESUMABLE_INCOMPLETE_STATUS_CODE &&
+      resp.headers.range;
+
+    if (shouldContinueWithNextMultiChunkRequest) {


There are a few places like this, where we have almost completely different logic for chunk upload, vs., regular upload, but share a method.

It makes me wonder if the implementation would read slightly cleaner if we had, export class Upload extends Pumpify in index.ts, as it is today, but then introduced an additional class, export class ChunkUpload extends Upload.

It might introduce a bit of repeated logic, but I'm curious if it would make the implementation even more elegant.

+1, I think separating out the logic more between the two upload types would definitely make the code easier to read.

Good point - I figured after the refactor to support multiple requests most of the logic would be the same with the exception of 1 parameter (upstreamIterator in startUploading) and 3 if statements (including this one).

I was hoping shouldContinueWithNextMultiChunkRequest would make it easier to understand what's happening, but seemed to have backfired.

As I said, not blocking for me, but if you have the time to experiment with the refactor, I'd be curoius what it looks like.

README.md

ddelgrosso1 · 2021-12-30T14:46:22Z

The only blocker for me, would be I'd like to advocate for the major bump to this library -- curious what others think.

I am onboard with making this a major version bump. I think this library doesn't see much usage as is. I don't think we will have to be porting many fixes backwards, which as I learned can be a painful exercise.

That said, if others object I want to just triple check that the API surface wasn't changed as a result of this refactor?

danielbankhead · 2021-12-30T15:44:15Z

Thanks for the feedback @bcoe, @ddelgrosso1, & @tritone - labeled this as breaking.

feat: Init multi chunk upload support

87f1af7

product-auto-label bot added the api: storage Issues related to the googleapis/gcs-resumable-upload API. label Dec 13, 2021

test: Fix existing tests by adding missing chunk stream

61a0f9d

danielbankhead requested review from shaffeeullah and ddelgrosso1 December 14, 2021 21:21

ddelgrosso1 reviewed Dec 14, 2021

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

d-goog added 3 commits December 19, 2021 23:29

feat: Add support for multi-chunk upload

0808058

- https://cloud.google.com/storage/docs/performing-resumable-uploads#chunked-upload

docs: Document chunkSize option

0b2eee3

refactor: Refactor, bug fixes, documentation, & clarification

31ffe46

d-goog added 7 commits December 21, 2021 14:11

refactor: Lint, bug fixes, and clarification

bc2afbe

style: Make linter happy

182ea29

refactor: Minor clean up and clarification

984a984

fix: Fix comp w/ Node 10

47fb952

API `writable.writableEnded` isn't available until Node 12.9.0

test: Update tests for multi chunk support and improve existing ones

1585586

build: Remove now unused 'is-stream'

3cddeb3

Merge branch 'main' of https://github.com/googleapis/gcs-resumable-up…

3458a97

…load into multi-upload-by-chunk-support

danielbankhead marked this pull request as ready for review December 23, 2021 19:32

danielbankhead requested review from a team as code owners December 23, 2021 19:32

danielbankhead requested a review from bcoe December 23, 2021 19:33

danielbankhead changed the title ~~feat: Init multi chunk upload support~~ feat: Multiple Chunk Upload Support Dec 23, 2021

ddelgrosso1 reviewed Dec 28, 2021

View reviewed changes

package.json Show resolved Hide resolved

ddelgrosso1 reviewed Dec 28, 2021

View reviewed changes

test/test.ts Show resolved Hide resolved

ddelgrosso1 reviewed Dec 28, 2021

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

tritone reviewed Dec 28, 2021

View reviewed changes

d-goog added 3 commits December 28, 2021 11:36

refactor: Use constants and methods for status checks

d24e9b8

docs: Clarification for chunkSize

b66aa70

docs: Clarifications in documentation

85b53fa

ddelgrosso1 reviewed Dec 28, 2021

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

ddelgrosso1 reviewed Dec 28, 2021

View reviewed changes

src/index.ts Show resolved Hide resolved

test: Pass status in responseHandler tests

2b6af40

ddelgrosso1 approved these changes Dec 29, 2021

View reviewed changes

bcoe suggested changes Dec 29, 2021

View reviewed changes

fix: Fix generic compatibility

f54eae4

tritone reviewed Dec 29, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

d-goog added 2 commits December 29, 2021 13:59

docs: Remove note on 'last chunk'

d7b6c59

fix: Fix type error from generic

0f3782f

bcoe approved these changes Dec 30, 2021

View reviewed changes

danielbankhead changed the title ~~feat: Multiple Chunk Upload Support~~ feat!: Multiple Chunk Upload Support Dec 30, 2021

danielbankhead merged commit dba1a39 into main Dec 30, 2021

danielbankhead deleted the multi-upload-by-chunk-support branch December 30, 2021 15:46

release-please bot mentioned this pull request Dec 30, 2021

chore: release 4.0.0 #488

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Multiple Chunk Upload Support #486

feat!: Multiple Chunk Upload Support #486

danielbankhead commented Dec 13, 2021 •

edited

Loading

ddelgrosso1 commented Dec 14, 2021

danielbankhead commented Dec 14, 2021

generated-files-bot bot commented Dec 21, 2021

ddelgrosso1 Dec 28, 2021

danielbankhead Dec 28, 2021

bcoe Dec 29, 2021

danielbankhead Dec 29, 2021

ddelgrosso1 commented Dec 28, 2021

tritone left a comment

tritone Dec 28, 2021

danielbankhead Dec 28, 2021

tritone Dec 29, 2021

danielbankhead Dec 29, 2021

tritone left a comment

ddelgrosso1 commented Dec 28, 2021

danielbankhead commented Dec 28, 2021

ddelgrosso1 left a comment

bcoe left a comment

bcoe Dec 29, 2021

bcoe Dec 29, 2021

danielbankhead Dec 29, 2021

bcoe Dec 29, 2021

bcoe Dec 29, 2021

danielbankhead Dec 29, 2021

bcoe Dec 29, 2021

tritone Dec 29, 2021

danielbankhead Dec 29, 2021

bcoe Dec 30, 2021

ddelgrosso1 commented Dec 30, 2021

danielbankhead commented Dec 30, 2021

feat!: Multiple Chunk Upload Support #486

feat!: Multiple Chunk Upload Support #486

Conversation

danielbankhead commented Dec 13, 2021 • edited Loading

ddelgrosso1 commented Dec 14, 2021

danielbankhead commented Dec 14, 2021

generated-files-bot bot commented Dec 21, 2021

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 commented Dec 28, 2021

tritone 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

tritone left a comment

Choose a reason for hiding this comment

ddelgrosso1 commented Dec 28, 2021

danielbankhead commented Dec 28, 2021

ddelgrosso1 left a comment

Choose a reason for hiding this comment

bcoe 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddelgrosso1 commented Dec 30, 2021

danielbankhead commented Dec 30, 2021

danielbankhead commented Dec 13, 2021 •

edited

Loading