-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add support for bucket to bucket sync of the latest versions of files #165
Conversation
This reverts commit 2f0f77f.
Co-authored-by: Paweł Polewicz <[email protected]>
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 is a review of just the first commit. (It's time for the next meeting, and I want the comments to be visible.)
Overall question: What are the semantics of copying from a hidden file? It looks like the destination file gets deleted. Is that what's expected?
@@ -687,10 +687,10 @@ def copy( | |||
""" | |||
|
|||
copy_source = CopySource(file_id, offset=offset, length=length) | |||
if length is None: | |||
if not length: |
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 check is not the same when length
is 0. I think this first branch should still happen in that case.
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.
The first branch is using a simple copy manager and the second emerger created by @mzukowski-reef
The problem is that emerger doesn't work when copying 0-length files and actually is not needed for such files
""" | ||
|
||
def _get_hide_delete_actions(self): | ||
for action in super()._get_hide_delete_actions(): |
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.
What actions might the parent class have that we want to use here? (It looks like the actual base class just returns the empty list.)
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.
If the parent class will start returning something, we'd like this class (and all descendants) to honor that without modification of the inheriting classes
if length is None and offset > 0: | ||
raise ValueError('Cannot copy with non zero offset and unknown length') | ||
if not length and offset > 0: | ||
raise ValueError('Cannot copy with non zero offset and unknown or zero length') |
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.
Why? Can't we copy a zero-length file?
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.
We could, but we'd rather not. Uploads are A-class transactions (free), but b2_copy_file
are C-class transactions (2500/day for free, then $0.004 per 1,000 calls), so since we are not going to save much bandwidth on transfer, it's better to re-upload the "whole" file.
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 is an emerger code. See my previous comment.
@@ -38,13 +38,15 @@ def is_copy(self): | |||
return True | |||
|
|||
def get_bytes_range(self): | |||
if self.length is None: | |||
if not self.length: |
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.
here, too
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.
Same here
I went through the rest of the commits, and they look fine. I do think there needs to be documentation on what happens when you sync from bucket to bucket. I think it does not copy all the versions of files, just the most recent one. And when in "delete" mode, when the source file is hidden, all versions of the destination file will be deleted. |
For me 1 and 2 are ok, but 3 is weird behavior but is not something that I introduced as it works exactly the same way when the destination is a local folder. |
Please file a separate issue for that so that we may decide whether this is a bug that needs to be fixed for local folders too |
Where would you like to see such documentation? In CLI? I think that overall sync documentation in b2sdk is less informative comparing to CLI sync command description. |
both, actually. Also, we'll add sync of all versions in a separare PR |
I've added a better docstring, updated the sphinx docs slightly, and created tickets for dealing with hidden files and sync of all versions (linked). |
An update for the CLI: Backblaze/B2_Command_Line_Tool#667 |
Co-authored-by: Paweł Polewicz <[email protected]>
Codacy says: apart from that I think we are ready to merge |
Fixed. |
An example of usage: