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

Fix broken SLA Mechanism #14056

Merged
merged 1 commit into from
Feb 4, 2021
Merged

Fix broken SLA Mechanism #14056

merged 1 commit into from
Feb 4, 2021

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Feb 4, 2021

closes #14050

We were not de-serializing BaseOperator.sla properly, hence
we were returning float instead of timedelta object.

Example: 100.0 instead of timedelta(seconds=100)

And because we had a check in _manage_sla in SchedulerJob and DagFileProcessor,
we were skipping SLA.

SchedulerJob:

if not any(isinstance(ti.sla, timedelta) for ti in dag.tasks):
self.log.debug("Skipping SLA check for %s because no tasks in DAG have SLAs", dag)
return

DagFileProcessor:

if not any(isinstance(ti.sla, timedelta) for ti in dag.tasks):
self.log.info("Skipping SLA check for %s because no tasks in DAG have SLAs", dag)
return

Tested it with breeze too: #14050 (comment)


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

closes apache#14050

We were not de-serializing `BaseOperator.sla` properly, hence
we were returning float instead of `timedelta` object.

Example: 100.0 instead of timedelta(seconds=100)

And because we had a check in _manage_sla in `SchedulerJob` and `DagFileProcessor`,
we were skipping SLA.

SchedulerJob:
https://github.com/apache/airflow/blob/88bdcfa0df5bcb4c489486e05826544b428c8f43/airflow/jobs/scheduler_job.py#L1766-L1768

DagFileProcessor:
https://github.com/apache/airflow/blob/88bdcfa0df5bcb4c489486e05826544b428c8f43/airflow/jobs/scheduler_job.py#L395-L397
@kaxil kaxil added this to the Airflow 2.0.1 milestone Feb 4, 2021
@boring-cyborg boring-cyborg bot added area:Scheduler including HA (high availability) scheduler area:serialization labels Feb 4, 2021
@github-actions
Copy link

github-actions bot commented Feb 4, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Feb 4, 2021
@kaxil kaxil merged commit 604a37e into apache:master Feb 4, 2021
@kaxil kaxil deleted the fix-broken-sla branch February 4, 2021 01:59
kaxil added a commit that referenced this pull request Feb 4, 2021
closes #14050

We were not de-serializing `BaseOperator.sla` properly, hence
we were returning float instead of `timedelta` object.

Example: 100.0 instead of timedelta(seconds=100)

And because we had a check in _manage_sla in `SchedulerJob` and `DagFileProcessor`,
we were skipping SLA.

SchedulerJob:
https://github.com/apache/airflow/blob/88bdcfa0df5bcb4c489486e05826544b428c8f43/airflow/jobs/scheduler_job.py#L1766-L1768

DagFileProcessor:
https://github.com/apache/airflow/blob/88bdcfa0df5bcb4c489486e05826544b428c8f43/airflow/jobs/scheduler_job.py#L395-L397
(cherry picked from commit 604a37e)
turbaszek added a commit to PolideaInternal/airflow that referenced this pull request Feb 4, 2021
Users have to provide SLA as timedelta. If not provided it should
be a default None value.

This change will result in run time errors if other type than
timedelta is passed as SLA. With this change spotting apache#14056
would be easier.
kaxil added a commit that referenced this pull request Feb 4, 2021
closes #14050

We were not de-serializing `BaseOperator.sla` properly, hence
we were returning float instead of `timedelta` object.

Example: 100.0 instead of timedelta(seconds=100)

And because we had a check in _manage_sla in `SchedulerJob` and `DagFileProcessor`,
we were skipping SLA.

SchedulerJob:
https://github.com/apache/airflow/blob/88bdcfa0df5bcb4c489486e05826544b428c8f43/airflow/jobs/scheduler_job.py#L1766-L1768

DagFileProcessor:
https://github.com/apache/airflow/blob/88bdcfa0df5bcb4c489486e05826544b428c8f43/airflow/jobs/scheduler_job.py#L395-L397
(cherry picked from commit 604a37e)
kaxil pushed a commit that referenced this pull request Feb 7, 2021
Users have to provide SLA as timedelta. If not provided it should
be a default None value.

This change will result in run time errors if other type than
timedelta is passed as SLA. With this change spotting #14056
would be easier.
kaxil added a commit to astronomer/airflow that referenced this pull request Feb 16, 2021
closes apache#14050

We were not de-serializing `BaseOperator.sla` properly, hence
we were returning float instead of `timedelta` object.

Example: 100.0 instead of timedelta(seconds=100)

And because we had a check in _manage_sla in `SchedulerJob` and `DagFileProcessor`,
we were skipping SLA.

SchedulerJob:
https://github.com/apache/airflow/blob/88bdcfa0df5bcb4c489486e05826544b428c8f43/airflow/jobs/scheduler_job.py#L1766-L1768

DagFileProcessor:
https://github.com/apache/airflow/blob/88bdcfa0df5bcb4c489486e05826544b428c8f43/airflow/jobs/scheduler_job.py#L395-L397
(cherry picked from commit 604a37e)
(cherry picked from commit 8f643b8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler including HA (high availability) scheduler area:serialization full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SLA mechanism does not work
2 participants