Skip to content

Comments

Fix sla checks being skipped when the later task instance is skipped#30523

Closed
doiken wants to merge 2 commits intoapache:mainfrom
doiken:continue-sla-check-regardless-of-skipped-ti
Closed

Fix sla checks being skipped when the later task instance is skipped#30523
doiken wants to merge 2 commits intoapache:mainfrom
doiken:continue-sla-check-regardless-of-skipped-ti

Conversation

@doiken
Copy link
Contributor

@doiken doiken commented Apr 7, 2023

In the following code, even though t1 does not become "success",
no SLA miss occurs because the state of the subsequent t1 becomes "skipped".

from datetime import timedelta
from time import sleep

import pendulum
from airflow import DAG
from airflow.exceptions import AirflowSkipException, AirflowFailException
from airflow.operators.python import PythonOperator
from airflow.utils.dates import days_ago


def f():
    if pendulum.now().minute % 2 == 0:
        raise AirflowSkipException("skip")
    else:
        sleep(80)
        raise AirflowFailException("fail")

with DAG(
    dag_id="example_sla",
    schedule_interval="* * * * *",
    start_date=days_ago(2),
    catchup=False,
) as dag:
    PythonOperator(task_id="t1", python_callable=f, sla=timedelta(seconds=70))

In this PR, we would like to correct only the following behavior

  • As-is
    • t1_1(state=other than success and skip): sla checks skipped by skipped t1_2
    • t1_2(state=skipped)
  • To-be
    • t1_1(state=other than success and skip): sla checked
    • t1_2(state=skipped)

I believe the state=skipped behavior expected in the following PR is preserved.
#3370

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Apr 7, 2023
Copy link
Member

Choose a reason for hiding this comment

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

Remove or_ if you want to leave only one condition.

@potiuk
Copy link
Member

potiuk commented Apr 22, 2023

I sm noy that familiar with the states here, But I am not sure if we want to do it, so I will defer to others, but we think about deprecating the whole SLA feature due to it's misbehaviours (see Draft AIP here https://cwiki.apache.org/confluence/display/AIRFLOW/%5BWIP%5D+AIP-57+SLA+as+a+DAG-Level+Feature) , so I am not sure if we want to invest into changing what we know is already pretty broken.

@doiken doiken force-pushed the continue-sla-check-regardless-of-skipped-ti branch from d8805bb to 77ad6c1 Compare April 24, 2023 00:37
@doiken
Copy link
Contributor Author

doiken commented Apr 24, 2023

Thank you for letting me know about your plans.

I have corrected your point for now.
I believe the unit test failure in Tests / MSSQL2017 has nothing to do with my fix.

@potiuk potiuk force-pushed the continue-sla-check-regardless-of-skipped-ti branch from 77ad6c1 to 2a482f3 Compare April 24, 2023 09:38
@potiuk
Copy link
Member

potiuk commented Apr 24, 2023

I rebased it now to account for fix to mssql, but I think this one needs input from others

@github-actions
Copy link

github-actions bot commented Jun 9, 2023

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

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jun 9, 2023
@doiken
Copy link
Contributor Author

doiken commented Jun 14, 2023

There is a conflicting file, but I will leave it as is for stale.
If there is a demand for this PR, please let me know so I can fix it.

@github-actions github-actions bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Jun 15, 2023
@github-actions
Copy link

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

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jul 31, 2023
@github-actions github-actions bot closed this Aug 6, 2023
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 stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants