Skip to content
This repository was archived by the owner on Mar 3, 2026. It is now read-only.

fix: resumable uploads should respect autoRetry & multipart uploads should correctly use preconditions#1779

Merged
ddelgrosso1 merged 22 commits intomainfrom
shaffeeullah/gcs-resumable-upload-refactor-1
Feb 16, 2022
Merged

fix: resumable uploads should respect autoRetry & multipart uploads should correctly use preconditions#1779
ddelgrosso1 merged 22 commits intomainfrom
shaffeeullah/gcs-resumable-upload-refactor-1

Conversation

@shaffeeullah
Copy link
Copy Markdown
Contributor

@shaffeeullah shaffeeullah commented Feb 4, 2022 •
edited
Loading

Start taking steps toward completing #1743 & bug fixes along the way

@shaffeeullah shaffeeullah requested review from a team February 4, 2022 18:36
@product-auto-label product-auto-label Bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Feb 4, 2022
shaffeeullah
shaffeeullah commented Feb 10, 2022
View reviewed changes
Comment thread src/gcs-resumable-upload/index.ts
Comment thread test/gcs-resumable-upload.ts
@shaffeeullah shaffeeullah changed the title chore: gcs-resumable-upload should use retry interface from storage fix: gcs-resumable-upload should use retry interface from storage Feb 15, 2022
@shaffeeullah shaffeeullah changed the title fix: gcs-resumable-upload should use retry interface from storage fix: resumable uploads should respect autoRetry Feb 15, 2022
@shaffeeullah shaffeeullah changed the title fix: resumable uploads should respect autoRetry fix: resumable uploads should respect autoRetry & multipart uploads should correctly use preconditions Feb 15, 2022
ddelgrosso1
ddelgrosso1 approved these changes Feb 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ddelgrosso1 ddelgrosso1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for refactoring this. Since I contributed a few minor things you may want another set of eyes to take a look as well.

@shaffeeullah
Copy link
Copy Markdown
Contributor Author

shaffeeullah commented Feb 15, 2022

LGTM. Thank you for refactoring this. Since I contributed a few minor things you may want another set of eyes to take a look as well.

Good call. @danielbankhead can you pls review as well?

danielbankhead
danielbankhead reviewed Feb 15, 2022
View reviewed changes
Comment thread src/bucket.ts Outdated
Comment on lines +639 to +640
instanceRetryValue?: boolean;
instancePreconditionOpts?: PreconditionOptions;
Copy link
Copy Markdown
Contributor

@danielbankhead danielbankhead Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we want these public? I usually default to private to avoid breaking changes in the future (say we want to change the type or variable name).

If we want them public, but we don't want anyone to modify, we could use readonly here.

Copy link
Copy Markdown
Contributor Author

@shaffeeullah shaffeeullah Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed instanceRetryValue back to private. instancePreconditionOpts needs to be public so we can modify it for the tests. @ddelgrosso1 are you okay with this? just want to make sure we very intentionally make this decision.

Copy link
Copy Markdown
Contributor

@ddelgrosso1 ddelgrosso1 Feb 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with this. Think I removed it to try out something and never put it back.

danielbankhead
danielbankhead approved these changes Feb 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@danielbankhead danielbankhead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, 1 nit

@shaffeeullah shaffeeullah added the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 15, 2022
@gcf-owl-bot gcf-owl-bot Bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Feb 15, 2022
@ddelgrosso1 ddelgrosso1 merged commit 1e72586 into main Feb 16, 2022
@ddelgrosso1 ddelgrosso1 deleted the shaffeeullah/gcs-resumable-upload-refactor-1 branch February 16, 2022 16:29
@release-please release-please Bot mentioned this pull request Feb 16, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ddelgrosso1 ddelgrosso1 ddelgrosso1 approved these changes

@danielbankhead danielbankhead danielbankhead approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: storage Issues related to the googleapis/nodejs-storage API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shaffeeullah @danielbankhead @ddelgrosso1