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

GCS uploading: disable chunked transfer encoding if contentLength is set #853

Conversation

stephenplusplus
Copy link
Contributor

Fixes #601

  • Review
  • Tests

With this change, when a user provides a contentLength property with the metadata to createWriteStream(), the upload will switch to non-chunked.

Because this is a stream, request wants to default to chunked transfer encoding. Normally with streams, we don't know the length of the content until it's fully received. However, when we do (see the use case in #601), there is an added expense for using a chunked transfer.

To accomplish this, some maybe not-so-pretty techniques were used. I wanted to open this PR before I go too much further to see if anyone knows of other options to get what we're after.

@stephenplusplus stephenplusplus added api: storage Issues related to the Cloud Storage API. don't merge labels Sep 8, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 8, 2015
@gmiroshnykov
Copy link

Oh, I thought it would be way easier to implement :(
I didn't know request doesn't allow streams with chunked: false multipart requests.
But it kinda makes sense as you have to do all the plumbing and figuring out individual parts' Content-Lengths on your own.

I'd say the increase in code complexity is not worth it.

Maybe instead it makes more sense to provide a new API for simple (single part) uploads and let a user set metadata in a separate request if it's needed?

@stephenplusplus
Copy link
Contributor Author

That makes sense. I just pushed an update:

// Disable multipart:
file.createWriteStream({
  simple: true
});

// Disable chunked:
file.createWriteStream({
  simple: true,
  metadata: {
    contentLength: 238497
  }
});

With simple: true, only metadata.contentType and metadata.contentLength are honored. If the user set others, they're just ignored.

This API change feels a little weird... ideally, the user doesn't have to study or care about the "simple", "multipart", and "resumable" upload techniques. If we were to just add a new method, I'd be worried that having multiple ways to do similar things would lead to choice overload and confusion.

What if we automatically used a simple upload and set the Content-Length header anytime the user provides metadata.contentLength and no other metadata properties than contentType-- basically, the second example above without simple: true. We would not allow the user to explicitly choose between simple or multipart; I see that more as an implementation detail for our library to cover up. We can just use our documentation to show an example of how we support non-chunked, and support more advanced needs that way.

// @callmehiphop thoughts?

@stephenplusplus
Copy link
Contributor Author

The tricky part for me to dance around is the metadata; if a user sets a contentLength, that doesn't mean they want to avoid chunked transfer encoding and drop the rest of their metadata. Having a setting for what upload technique to use feels a bit too advanced and still doesn't lead to a clean implementation.

I'm thinking this might be a little too far on the micro-optimization side for our "easy to approach" library to support. I'm going to close this for now, but if anyone can argue otherwise or provide a more straightforward solution than I could come up with, please discuss here or on #601.

sofisl pushed a commit that referenced this pull request Nov 11, 2022
…recognition result docs: update environment docs (#853)

* fix: fix validation result docs feat: add language code to streaming recognition result docs: update environment docs

PiperOrigin-RevId: 388737005

Source-Link: googleapis/googleapis@8dfb215

Source-Link: googleapis/googleapis-gen@ba1df8d

* 🦉 Updates from OwlBot

See https://github.com/googleapis/repo-automation-bots/blob/master/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this pull request Nov 11, 2022
🤖 I have created a release \*beep\* \*boop\*
---
### [4.1.4](https://www.github.com/googleapis/nodejs-dialogflow/compare/v4.1.3...v4.1.4) (2021-08-04)


### Bug Fixes

* fix validation result docs feat: add language code to streaming recognition result docs: update environment docs ([#853](https://www.github.com/googleapis/nodejs-dialogflow/issues/853)) ([d2eb1bd](https://www.github.com/googleapis/nodejs-dialogflow/commit/d2eb1bdaf886493952163bcb41291ad492a7bfb8))
* fix validation result docs feat: add language code to streaming recognition result feat: add time zone and security settings to conversation profile docs: update agent docs docs: update entity type docs docs: update intent docs ([#854](https://www.github.com/googleapis/nodejs-dialogflow/issues/854)) ([cda7ff3](https://www.github.com/googleapis/nodejs-dialogflow/commit/cda7ff335abad989612c104e990be1f6a55c89b5))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
sofisl pushed a commit that referenced this pull request Jan 17, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Benjamin E. Coe <bencoe@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants