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

Fixed thread local _sentinel.callers defect and added test cases #44646

Merged
merged 26 commits into from
Dec 11, 2024

Conversation

rahulgoyal2987
Copy link
Contributor


^ 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.

@ashb
Copy link
Member

ashb commented Dec 5, 2024

Please add a meaningful description to this pr, and likely unit tests too

Copy link
Contributor Author

@rahulgoyal2987 rahulgoyal2987 left a comment

Choose a reason for hiding this comment

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

PFA

@rahulgoyal2987 rahulgoyal2987 changed the title Airflow sentinel defect Fixed thread local _sentinel.callers defect and added test cases Dec 6, 2024
Copy link
Contributor Author

@rahulgoyal2987 rahulgoyal2987 left a comment

Choose a reason for hiding this comment

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

PFA

airflow/models/baseoperator.py Show resolved Hide resolved
@dabla
Copy link
Contributor

dabla commented Dec 11, 2024

Is there a reason why this PR isn't merged yet as it fixes a thread safety issues with the ExecutorSafeguard?

@potiuk
Copy link
Member

potiuk commented Dec 11, 2024

I thi k we mostly overlooked it :(

@potiuk potiuk merged commit a77fca2 into apache:main Dec 11, 2024
49 checks passed
ellisms pushed a commit to ellisms/airflow that referenced this pull request Dec 13, 2024
…che#44646)

* Update base.py

* Update base.py

* Update base.py

* Update baseoperator.py

* Update base.py

* Update base.py

* Update baseoperator.py

* Update baseoperator.py

* Fixed thread local _sentinel.callers defect and added test cases

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

---------

Co-authored-by: Rahul Goyal <[email protected]>
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
…che#44646)

* Update base.py

* Update base.py

* Update base.py

* Update baseoperator.py

* Update base.py

* Update base.py

* Update baseoperator.py

* Update baseoperator.py

* Fixed thread local _sentinel.callers defect and added test cases

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

---------

Co-authored-by: Rahul Goyal <[email protected]>
@kulkarni-sp
Copy link

What is the target release version for this fix?

@potiuk
Copy link
Member

potiuk commented Jan 9, 2025

Currently Airflow 3. Why? Is this worthwhile to make an attempt to backport it to Airflow 2.* ? If so - why? What would be your arguments @kulkarni-sp ?

@kulkarni-sp
Copy link

Currently Airflow 3. Why? Is this worthwhile to make an attempt to backport it to Airflow 2.* ? If so - why? What would be your arguments @kulkarni-sp ?

We are currently using Airflow 2.9.3 and encountered the same errors during our attempt to upgrade to 2.10.3. Therefore, this fix is essential to unblock our upgrade path.

@potiuk
Copy link
Member

potiuk commented Jan 9, 2025

We are currently using Airflow 2.9.3 and encountered the same errors during our attempt to upgrade to 2.10.3. Therefore, this fix is essential to unblock our upgrade path.

Could you please be more specific - what exactly error you experienced - i think that one has no clear issue that it marks as "solving" ? And is it possible that you apply that patch to verify that this one solves it ?

@kulkarni-sp
Copy link

We are currently using Airflow 2.9.3 and encountered the same errors during our attempt to upgrade to 2.10.3. Therefore, this fix is essential to unblock our upgrade path.

Could you please be more specific - what exactly error you experienced - i think that one has no clear issue that it marks as "solving" ? And is it possible that you apply that patch to verify that this one solves it ?

We are utilizing Airflow with KubernetesExecutor and have several long-running pods that continuously monitor specified locations for input files. Initially, we used mapped tasks to specify different input locations, but this led to a higher number of pods and caused stability issues on our AKS cluster. Now, we are employing ThreadPoolExecutor for parallel processing, which triggers the processor DAG upon receiving a valid input file. However, with Airflow 2.10.3, we encounter the following errors when triggering the processor DAG:

[2025-01-10, 12:53:29 UTC] {ThreadPoolExecutor-1_3 logging_mixin.py:190} INFO - pa: Error while calling processor DAG: '_thread._local' object has no attribute 'callers'

@task(executor_config=k8s_poller_exec_config_resource_requirements)
    def poll_adls_driver(config_list, **kwargs):
        args_list = []

        for upstream_config in config_list:
            args_list.append((upstream_config, kwargs,))

        with ThreadPoolExecutor(max_workers=5) as executor:
            # Unpack the argument tuples
            futures = [executor.submit(poll_adls, *args) for args in args_list]
        for future in futures:
            future.result()

@kulkarni-sp
Copy link

kulkarni-sp commented Jan 15, 2025

@potiuk We have also applied this patch, Requires minor change for it to work in multithreaded env i.e. getattr check and initialization should be done before sentinel check OR It could work with this PR https://github.com/apache/airflow/pull/44240/files

        sentinel_key = f"{self.__class__.__name__}__sentinel"
        sentinel = kwargs.pop(sentinel_key, None)
        //Initialize attribute callers
         if not getattr(cls._sentinel, "callers", None):
            cls._sentinel.callers = {}
        if sentinel:                
            cls._sentinel.callers[sentinel_key] = sentinel
        else:                
            sentinel = cls._sentinel.callers.pop(f"{func.__qualname__.split('.')[0]}__sentinel", None)
        ----

@eladkal
Copy link
Contributor

eladkal commented Jan 28, 2025

I guess this issue fixes #44648 which is a bug on 2.10.3
@utkarsharma2 can we backport the fix to v2-10 branch?

@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Jan 28, 2025
@potiuk potiuk added this to the Airflow 2.10.5 milestone Jan 29, 2025
@potiuk
Copy link
Member

potiuk commented Jan 29, 2025

I marked it as 2.10.5 milestone to not forget about it.

@kulkarni-sp
Copy link

@utkarsharma2 Is it possible for you to include this make minor change? Essentially doing getattr check before if statement.
//Initialize attribute callers
if not getattr(cls._sentinel, "callers", None):
cls._sentinel.callers = {}
if sentinel:
cls._sentinel.callers[sentinel_key] = sentinel
else:
sentinel = cls._sentinel.callers.pop(f"{func.qualname.split('.')[0]}__sentinel", None)
----

utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Jan 30, 2025
…che#44646)

* Update base.py

* Update base.py

* Update base.py

* Update baseoperator.py

* Update base.py

* Update base.py

* Update baseoperator.py

* Update baseoperator.py

* Fixed thread local _sentinel.callers defect and added test cases

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

---------

Co-authored-by: Rahul Goyal <[email protected]>
(cherry picked from commit a77fca2)
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
…che#44646)

* Update base.py

* Update base.py

* Update base.py

* Update baseoperator.py

* Update base.py

* Update base.py

* Update baseoperator.py

* Update baseoperator.py

* Fixed thread local _sentinel.callers defect and added test cases

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

---------

Co-authored-by: Rahul Goyal <[email protected]>
utkarsharma2 added a commit that referenced this pull request Jan 30, 2025
) (#46280)

* Update base.py

* Update base.py

* Update base.py

* Update baseoperator.py

* Update base.py

* Update base.py

* Update baseoperator.py

* Update baseoperator.py

* Fixed thread local _sentinel.callers defect and added test cases

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

* Fixed issue

---------

Co-authored-by: Rahul Goyal <[email protected]>
(cherry picked from commit a77fca2)

Co-authored-by: rahulgoyal2987 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants