Skip to content

[AIRFLOW-6852] Force reschedule mode for sensors when running on SequentialExecutor#7473

Closed
ANiteckiP wants to merge 1 commit intoapache:masterfrom
ANiteckiP:AIRFLOW-6852-force-reschedule-mode-for-sensors-on-SequentialExecutor
Closed

[AIRFLOW-6852] Force reschedule mode for sensors when running on SequentialExecutor#7473
ANiteckiP wants to merge 1 commit intoapache:masterfrom
ANiteckiP:AIRFLOW-6852-force-reschedule-mode-for-sensors-on-SequentialExecutor

Conversation

@ANiteckiP
Copy link
Contributor

@ANiteckiP ANiteckiP commented Feb 20, 2020

Force reschedule mode for sensors when running on SequentialExecutor to avoid deadlocks.

Issue link: AIRFLOW-6852

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

@turbaszek
Copy link
Member

Can you check how this will work with the following change?
#7197

Stateful sensors should not be used in reschedule mode.

@ANiteckiP
Copy link
Contributor Author

ANiteckiP commented Feb 20, 2020

@nuclearpinguin This change would effectively prohibit using poke-only sensors together with SequentialExecutor - SequentialExecutor would force the reschedule mode and the code in PR you linked would cause DAG parsing to fail.

@turbaszek
Copy link
Member

I am not convinced we should do this change. The SequentialExecutor can be used by users in production (I hope I'm wrong!) and this may cause some problems. @potiuk @ash any thoughts?

@turbaszek turbaszek requested review from ashb and potiuk February 22, 2020 10:43
@ashb
Copy link
Member

ashb commented Feb 24, 2020

The SequentialExecutor stops heartbeating when running a task, so this is very very unlikely to be used in production, as you'd constantly get "The scheduler is not running" notices in the UI.

I'm not even sure we should have Sequential executor anymore. LocalExecutor with a single process would be much much better.

if isinstance(task_copy, BaseSensorOperator) and \
conf.get('core', 'executor') == "DebugExecutor":
conf.get('core', 'executor') in ["DebugExecutor", "SequentialExecutor"]:
self.log.warning("DebugExecutor changes sensor mode to 'reschedule'.")
Copy link
Member

Choose a reason for hiding this comment

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

This log needs updating anyway :)

@potiuk
Copy link
Member

potiuk commented Feb 24, 2020

The SequentialExecutor stops heartbeating when running a task, so this is very very unlikely to be used in production, as you'd constantly get "The scheduler is not running" notices in the UI.

I'm not even sure we should have Sequential executor anymore. LocalExecutor with a single process would be much much better.

SequentialExecutor is currently the only one allowed for SQLIte

    def _validate(self):
        if (
                self.get("core", "executor") not in ('DebugExecutor', 'SequentialExecutor') and
                "sqlite" in self.get('core', 'sql_alchemy_conn')):
            raise AirflowConfigException(
                "error: cannot use sqlite with the {}".format(
                    self.get('core', 'executor')))

But indeed, if we change it to enforce the LocalExecutor and count ==1 this might work and we can get rid of the Sequential executor.

@potiuk
Copy link
Member

potiuk commented Feb 24, 2020

And I've heard anecdotal evidence of people using sqlite in production. Unluckily we haven't asked for the database in the survey :(

@stale
Copy link

stale bot commented Apr 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Apr 9, 2020
@stale stale bot closed this Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments