-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Fix unclosed aiohttp ClientSession in AzureDataFactoryAsyncHook #60650
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
Fix unclosed aiohttp ClientSession in AzureDataFactoryAsyncHook #60650
Conversation
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.
Looks good to me. Turning the async hook into a context manager and updating the trigger call sites fixes the aiohttp client leak cleanly. Tests look solid as well.
Not a blocker but I would suggest double checking if there any places outside this file where this hook is being called so that the leak does not persist after your fix.
|
@SameerMesiah97 Double checked other places, looks good. |
providers/microsoft/azure/src/airflow/providers/microsoft/azure/triggers/data_factory.py
Outdated
Show resolved
Hide resolved
providers/microsoft/azure/src/airflow/providers/microsoft/azure/triggers/data_factory.py
Outdated
Show resolved
Hide resolved
providers/microsoft/azure/src/airflow/providers/microsoft/azure/triggers/data_factory.py
Outdated
Show resolved
Hide resolved
providers/microsoft/azure/src/airflow/providers/microsoft/azure/triggers/data_factory.py
Show resolved
Hide resolved
providers/microsoft/azure/src/airflow/providers/microsoft/azure/triggers/data_factory.py
Outdated
Show resolved
Hide resolved
dabla
left a comment
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.
2 small nits but looking good!
providers/microsoft/azure/src/airflow/providers/microsoft/azure/triggers/data_factory.py
Outdated
Show resolved
Hide resolved
providers/microsoft/azure/src/airflow/providers/microsoft/azure/triggers/data_factory.py
Outdated
Show resolved
Hide resolved
dabla
left a comment
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.
Looking good to me.
|
@cruseakshay Did you actually tests this changes and can you confirm this works? |
|
@cruseakshay Also, the changes made in the data_factory trigger, like the added _build_trigger_event method like I asked, would be nice to see a test for it as well. Is the trigger data_factory tests also testing the context manger? |
Yes, here are trail logs from latest run |
hook-level test that covers the context manager behavior: |
|
@cruseakshay Thanks for the PR and your patience, good job! |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
…he#60650) * Fix unclosed aiohttp ClientSession in AzureDataFactoryAsyncHook * If cancel_pipeline_run() creates a new connection * Address review comments * review: early return * add tests: build_trigger_event --------- Co-authored-by: Akshay <cruseakshay@users.noreply.github.com>
Closes: #55724
Fix unclosed
aiohttp.ClientSessionandTCPConnectorwarnings when usingAzureDataFactoryRunPipelineOperatorwithdeferrable=True.The
AzureDataFactoryAsyncHookcreates anAsyncDataFactoryManagementClientbut never closes it, causing resource leak errors in logs regardless of pipeline success/failure.Changes
__aenter__,__aexit__, andclose()methods toAzureDataFactoryAsyncHookasync with hook:for proper cleanupFollows the same pattern as #52119 (Databricks fix).
Was generative AI tooling used to co-author this PR?
Generated-by: Claude following the guidelines