-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Use sync instead of copy for blob storage #6298
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
Conversation
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)) |
There was a problem hiding this comment.
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', []), |
There was a problem hiding this comment.
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.
readthedocs.org/readthedocs/rtd_tests/tests/test_build_storage.py
Lines 39 to 41 in 43463b4
# 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
There was a problem hiding this 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!
There was a problem hiding this 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?
No completely, we still need to remove files when a version is deactivated, but I'm making another PR about that. |
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