Skip to content
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

Close open connections for deferrable SFTPSensor #38881

Merged

Conversation

pankajkoti
Copy link
Member

@pankajkoti pankajkoti commented Apr 9, 2024

It's observed that running the SFTPSensor in deferrable mode
fails to close opened SSH connections to the server, resulting
in a continuous increase of open connections over time.
This pull request addresses this issue by ensuring that
connections are opened using a context manager, allowing
them to be closed once the relevant data has been read
from the SSH server during file sensing.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@pankajkoti
Copy link
Member Author

pankajkoti commented Apr 9, 2024

Before this PR, we can see in task logs that there was no close connection called
Screenshot 2024-04-10 at 1 44 15 AM

With this PR, we do send close connection calls
Screenshot 2024-04-10 at 1 44 56 AM

Before this PR, on the SSH server, we can see growing number of connections that just logs that it's accepting connections, but no connection closures received
Screenshot 2024-04-10 at 1 43 57 AM

With this PR, we see connection closures being received and that the count does not keep on accumulating
Screenshot 2024-04-10 at 1 30 13 AM

Copy link
Contributor

@fritz-astronomer fritz-astronomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good, I think it is possible to test it by checking if the method __aexit__ (or close if it has one) is called.

However, I wonder why you need # type: ignore[return]? 🤔

@Lee-W Lee-W force-pushed the close-ssh-connection-deferrable-sftp branch from 5c0bd70 to 119fd3b Compare April 10, 2024 02:10
@Lee-W
Copy link
Member

Lee-W commented Apr 10, 2024

The change looks good, I think it is possible to test it by checking if the method __aexit__ (or close if it has one) is called.

Yep, this is what we should check and is part of the reason why the tests failed originally. Just fixed it 🔧

However, I wonder why you need # type: ignore[return]? 🤔

Otherwise, we'll encounter error: Missing return statement [return]. Tried a few ways, but didn't find a good enough way to pass it 🤔

@Lee-W Lee-W marked this pull request as ready for review April 10, 2024 02:56
@Lee-W
Copy link
Member

Lee-W commented Apr 10, 2024

I also test it locally to verify the functionality. 👍

Copy link
Collaborator

@sunank200 sunank200 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@phanikumv phanikumv merged commit d703a22 into apache:main Apr 10, 2024
40 checks passed
@phanikumv phanikumv deleted the close-ssh-connection-deferrable-sftp branch April 10, 2024 07:36
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants