AP-724: Add resume support for interrupted file transfers#85
Conversation
|
Before digging too deeply into the code it looks like a lot of this is a refactor and not related to the ticket unless I'm missing something. Should the refactor be a separate merge request or is it required for resume support? |
anarchivist
left a comment
anarchivist
left a comment
There was a problem hiding this comment.
r+; generally looks good to me. (tried to run it locally and ran out of disk space 😅)
The changes in copy_files.py is pretty much just refactoring. They could be a separate PR, but not really sure it's worth the effort at this point. |
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
r+wc: A few style nits, but overall really well done!
|
|
||
|
|
There was a problem hiding this comment.
We don't need this many blank lines. (I'm surprised nothing complained?)
There was a problem hiding this comment.
I noticed that one too right after I put in the PR...surprised as well whatever linter we're using didn't kick and scream about that one! (Fixed!)
| monkeypatch.setattr( | ||
| file_transfer.shutil, | ||
| "copy2", | ||
| real_copy2, | ||
| ) | ||
|
|
There was a problem hiding this comment.
This seems to be a "dead store": the value is never used before being set again at line 441.
| if not destination_path.is_dir(): | ||
| raise AirflowFailException(f"Destination is not a directory: {destination_path}") | ||
|
|
||
| if next(os.scandir(destination_path), None) is not None: |
There was a problem hiding this comment.
minor, since we're using pathlib quite a bit and destination_path is a Path might want to stick with that.
| if next(os.scandir(destination_path), None) is not None: | |
| if any(destination_path.iterdir()) |
There was a problem hiding this comment.
I had that previously but Anna pointed out that the performance would be an issue:
"iterdir() is correct, but os.scandir() is faster, especially on NFS."
I'd lean on performance over style here!
There was a problem hiding this comment.
Ah, I see that now. Looks like iterdir() reads everything in at once which can cause a bottleneck. Looks like next(destination_path.glob("*"), None) is not None: would os.scandir under the hood with the pathlib approach so probably the same performance.
pretty nitpicky though
anarchivist
left a comment
anarchivist
left a comment
There was a problem hiding this comment.
follow up r+ after chcanges.
4 participants
No description provided.