Delay the creation of ssh proxy until get_conn()
#20474
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kaxil
reviewed
Dec 23, 2021
uranusjr
reviewed
Dec 23, 2021
kaxil
reviewed
Dec 23, 2021
uranusjr
approved these changes
Dec 24, 2021
|
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
3093e54 to
54b6d15
Compare
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: Kaxil Naik <kaxilnaik@apache.org>
54b6d15 to
54939b4
Compare
|
I rebased and squashed the changes, expecting that's a required step (let me know if it's not the case). Please let me know if there are more things to do before the merge. Thanks. |
|
sorry to nudge but what else should be done before the merge? thanks. |
potiuk
approved these changes
Jan 9, 2022
|
Thank you! |
74 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We recently upgraded from Airflow 1.9 to 2.2.2 (to be more precise, we are still in the process of upgrading, dealing with various issues related to the upgrade). I like the change that the newly refactored SSHHook initializes most of the parameters in
__init__, making customization easier. However, there is one "heavy" operation related to SSH proxy which I wonder might be better off moving back to where it will be used.I noticed that for every 30+ seconds (
min_file_process_interval=30), during the run of DAGFileProcessor, a bunch of SSH processes would be launched. This is because aparamiko.ProxyCommandis ran for any connection used in tasks, as long as that connection has a "ProxyCommand" in the.ssh/configfile. This seems too much (also heavy) to me.I wonder if a change in the spirit of the attached pull request is acceptable or not?