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

[WIP] Use rename in blob upload #3676

Closed
wants to merge 3 commits into from
Closed

[WIP] Use rename in blob upload #3676

wants to merge 3 commits into from

Conversation

dragotin
Copy link
Contributor

Use rename instead of copy data again for the cost of an less clean interface of BlobStore. @aduffeck

@dragotin dragotin requested review from a team, labkode and ishank011 as code owners February 22, 2023 10:23
@update-docs
Copy link

update-docs bot commented Feb 22, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.


// ensure parent path exists
if err := os.MkdirAll(filepath.Dir(bs.path(node)), 0700); err != nil {
return errors.Wrap(err, "Decomposedfs: oCIS blobstore: error creating parent folders for blob")
}

f, err := os.OpenFile(bs.path(node), os.O_CREATE|os.O_WRONLY, 0700)
// check that destination is not a directory, that makes Rename fail
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you actually run into the problem that the blob already exists as a directory? That would be somewhat worrisome because the blobstore storage should be controlled by the blobstore itself and it only creates files for the blobs, with the name being derived from the blobid of the node.

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise I think I would rather omit the special handling for the non-expected case, knowing that the code writing the blob will fail anyway when trying to write to a directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removed the directory check.

@butonic
Copy link
Contributor

butonic commented Mar 21, 2023

closing in favor of #3739

@butonic butonic closed this Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants