Add deferrable mode to SFTPOperator#68298
Conversation
|
I’m opening a new PR as a continuation/replacement for accidentally closed PR #65480. While rebasing and cleaning up the branch history against upstream/main, I mistakenly performed a reset/rebase sequence that rewrote the branch history and caused GitHub to show “0 commits,” which made the original PR impossible to reopen properly. This was completely unintentional, and I sincerely apologize for the confusion and extra noise caused during review. Over the past couple of months, I worked extensively on this contribution — implementing deferrable support for Thankfully, the commits were still recoverable through Thank you again for all the reviews, feedback, patience, and guidance throughout this process. I truly appreciate the maintainers and reviewers taking another look at the contribution 🙏 |
|
@dabla @potiuk — the "200 changed files" in the GitHub Files tab The actual diff vs apache:main is only 11 files: providers/sftp/src/.../constants.py |
|
You could have simply renamed your old corrupt branch, and created a new branch with same name and force pushed it, that way the existing PR (with review and comments) would have been kept, which would make review for us easier. Now the context is unfortunately lost when creating a new PR, and we reviewers, have to check back with original PR. So please take that into consideration for the future, everyone makes mistakes me included, have encountered the same myself multiple times, that’s how I know the above trick is ideal in such situations. |
|
@dabla — thank you for the explanation and guidance. That makes complete sense now, and I really appreciate you sharing the branch recovery approach. I wasn’t aware that recreating the branch with the same name would preserve the original PR context and review history. I’ll definitely keep this workflow in mind going forward. Apologies again for the inconvenience and extra review overhead caused by the rewritten branch history. Thank you again for the patience and for continuing to review the contribution 🙏 |
|
@dabla @potiuk @srchilukoori — I'm aware of PR #68520 implementing I've now fixed the branch cleanly — single commit, correct diff I respectfully ask that this PR be reviewed ahead of #68520 given |
- Add deferrable=True parameter to SFTPOperator - Implement SFTPTrigger for async file transfers via asgiref sync_to_async - Add transfer() method to SFTPHook and SFTPHookAsync (DRY principle) - Operator and trigger delegate to hook.transfer() - Add asgiref>=3.5.2 dependency - Add newsfragment and update docs requirements table - Add tests for deferrable mode and SFTPTrigger Continuation of work from PR apache#65480
- Add deferrable=True parameter to SFTPOperator - Implement SFTPTrigger for async file transfers via asgiref sync_to_async - Add transfer() method to SFTPHook and SFTPHookAsync (DRY principle) - Operator and trigger delegate to hook.transfer() - Add asgiref>=3.5.2 dependency - Add newsfragment and update docs requirements table - Add tests for deferrable mode and SFTPTrigger
9687b3f to
7602ac3
Compare
|
@dabla @potiuk — branch fixed. All files now present and clean: Given the 3+ months of review history already incorporated here, |
…, trigger delegates directly to hook
|
@dabla — both addressed:
Newsfragment renamed to match this PR (68298.feature.rst). Ready for re-review 🙏 |
Do not forget to resolve comments once addressed... |
…ion concurrent transfer() with asyncio.gather, use __name__ for method_name
|
@dabla — all 3 addressed:
Ready for re-review 🙏 |
…ithin_directory calls
be85bed to
d926176
Compare
|
@ashb @dabla @potiuk @bugraoz93 @gopidesupavan @amoghrajesh @jason810496 @jscheffl Thank you for the detailed feedback! I'm addressing the async optimization suggestions from @dabla. I'll update the implementation to:
Pushing updates shortly. Appreciate the thorough review! |
bugraoz93
left a comment
There was a problem hiding this comment.
Thanks for the PR and updates! I have two things
One is package version. The other is enum approach. After finishing and seeing even their values are not used nor regenerated this should be definitely enum. If others hold, dataclass
|
@bugraoz93 Thank you for the detailed review I've addressed all your concerns: Issue 1: asgiref Version Bump
|
Closes: #65475
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.