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.

Sign up for GitHub

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

Jump to bottom

fix!: remove configstore #1871

Merged
merged 3 commits into from Apr 15, 2022
Merged

fix!: remove configstore #1871

merged 3 commits into from Apr 15, 2022

Conversation

shaffeeullah
Copy link
Contributor

@shaffeeullah shaffeeullah requested review from a team as code owners April 13, 2022 22:44
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: storage Issues related to the googleapis/nodejs-storage API. labels Apr 13, 2022
danielbankhead
danielbankhead reviewed Apr 14, 2022
View reviewed changes
Copy link
Member

@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.

Such a therapeutic PR

src/gcs-resumable-upload.ts Show resolved Hide resolved
@danielbankhead
Copy link
Member

I think xdg-basedir can be removed in this PR too

@ddelgrosso1 ddelgrosso1 merged commit c0e2bb9 into dropNode10 Apr 15, 2022
@ddelgrosso1 ddelgrosso1 deleted the shaffeeullah/goodbyeConfigstore2 branch April 15, 2022 14:23
@shaffeeullah shaffeeullah mentioned this pull request Apr 15, 2022
Closed
@FossPrime FossPrime mentioned this pull request Apr 20, 2022
Closed
ddelgrosso1 pushed a commit to ddelgrosso1/nodejs-storage that referenced this pull request May 23, 2022
@shaffeeullah @ddelgrosso1
fix!: remove configstore (googleapis#1871)
a6205ba 
* fix: remove configstore

* removed unused varaibles

* remove xdg-basedir
@rhodgkins
Copy link
Contributor

Hi with this change (and along with #1870 / #1876) how do resumable uploads now work?
Asking as when running on Cloud Run (see #1133), you have set the XDG_ envars for resumable uploads to work (or configPath) - is all this no longer needed?

@ddelgrosso1
Copy link
Contributor

Hi @rhodgkins you are correct, with the removal of config store as a requirement any environment variables previously used to set the path for config store to write to are no longer necessary.

@rhodgkins
Copy link
Contributor

Thanks @ddelgrosso1, out of interest how do resumable uploads work now? Are they only resumable for the lifetime of the nodejs process?

@ddelgrosso1
Copy link
Contributor

@rhodgkins apologies for the delay in responding. Yes that is correct they are only resumable during the lifetime of a process now. We realized during our investigation into whether or not configStore was needed that there existed the possibility of data corruption in the old methodology. ConfigStore was only being used to store the first 16 bytes of an uploaded file which is a poor way to determine if the files are the same. Coupled with the numerous issues with having to cache on disk we made the decision to remove configStore entirely. Now, during the lifetime of a process a resumable upload will resume from the spot of interruption.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielbankhead danielbankhead danielbankhead approved these changes

@ddelgrosso1 ddelgrosso1 ddelgrosso1 approved these changes

Assignees
No one assigned
Labels
api: storage Issues related to the googleapis/nodejs-storage API. size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@shaffeeullah @danielbankhead @rhodgkins @ddelgrosso1