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

Use sync instead of copy for blob storage #6298

Merged
merged 3 commits into from
Oct 22, 2019

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Oct 16, 2019

We use rsync for our normal storage, but for blob storage we are just doing a copy.

Full explanation at #6186 (comment)

This is heavily borrowed from a snippet from @davidfischer :)

Fixes #6069
Closes #6186

@stsewd stsewd requested review from davidfischer and a team October 16, 2019 19:09
dest_folders, dest_files = self.listdir(self._dirpath(destination))
for folder in dest_folders:
if folder not in copied_dirs:
self.delete_directory(self.join(destination, folder))
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this, because set1 - set2 creates another set. not in is O(1).


tree = [
# Cloud storage generally doesn't consider empty directories to exist
('api', []),
Copy link
Member Author

Choose a reason for hiding this comment

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

I was about to fix this to delete all dirs on FileSystemStorage, but looks like we expect that and probably isn't a big deal.

# We don't check "dirs" here - in filesystem backed storages
# the empty directories are not deleted
# Cloud storage generally doesn't consider empty directories to exist

- Linting fix
- Check against suspicious operations
- Additional logging
Copy link
Contributor

@davidfischer davidfischer left a comment

Choose a reason for hiding this comment

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

I fixed the linting error and added a few other cleanup parts. This is good to go and should fix the lingering storage issues. Thanks for doing this @stsewd!

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This makes sense, and I'm guessing will help fix the lingering issues we have with files sticking around after deletion?

@stsewd
Copy link
Member Author

stsewd commented Oct 22, 2019

This makes sense, and I'm guessing will help fix the lingering issues we have with files sticking around after deletion?

No completely, we still need to remove files when a version is deactivated, but I'm making another PR about that.

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.

Search index not updated after renaming/moving of a page
3 participants