You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/10/10 12:27:09 UTC

[GitHub] [airflow] doiken opened a new pull request, #26968: sla: continue checking sla even if the preceding task instance misses the sla

doiken opened a new pull request, #26968:
URL: https://github.com/apache/airflow/pull/26968

   If the preceding running task instance misses the sla,
   sla check for subsequent task instances is skipped.
   
   We want to continue checking them.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] doiken commented on pull request #26968: Fix sla checks being skipped when the preceding task instance misses the sla

Posted by GitBox <gi...@apache.org>.
doiken commented on PR #26968:
URL: https://github.com/apache/airflow/pull/26968#issuecomment-1350336810

   It has been about a month since the first approve.
   Is there anything I should do?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] doiken commented on a diff in pull request #26968: Fix sla checks being skipped when the preceding task instance misses the sla

Posted by GitBox <gi...@apache.org>.
doiken commented on code in PR #26968:
URL: https://github.com/apache/airflow/pull/26968#discussion_r1013564570


##########
airflow/dag_processing/processor.py:
##########
@@ -425,7 +425,7 @@ def manage_slas(self, dag: DAG, session: Session = None) -> None:
                 if next_info is None:
                     break
                 if (ti.dag_id, ti.task_id, next_info.logical_date) in recorded_slas_query:
-                    break
+                    continue

Review Comment:
   > This seems like a change in behaviour
   
   I would like not to change but fix the behaviour changed by https://github.com/apache/airflow/pull/19553.
   
   I think the current sla check behaviour is split as follows
   - No SlaMiss record: all task instances missing sla are recorded
   - SlaMiss record already exists: no task instance is recorded by `break`
   
   > It seems like it's going to record too many SLAs this way.
   
   Only tasks satisfying `next_info.logical_date + task.sla < ts` are recorded.
   
   If we want to `break` here,
   I think we need break after first SlaMiss is appended as well to keep behaviour consistent.
   ```
   ...
   sla_miss = SlaMiss(
       task_id=ti.task_id,
       dag_id=ti.dag_id,
       execution_date=next_info.logical_date,
       timestamp=ts,
   )
   sla_misses.append(sla_miss)
   Stats.incr('sla_missed')
   break
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #26968: Fix sla checks being skipped when the preceding task instance misses the sla

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26968:
URL: https://github.com/apache/airflow/pull/26968#issuecomment-1296379870

   Needs conflict resolving I am afraid. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk merged pull request #26968: Fix sla checks being skipped when the preceding task instance misses the sla

Posted by GitBox <gi...@apache.org>.
potiuk merged PR #26968:
URL: https://github.com/apache/airflow/pull/26968


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] doiken commented on pull request #26968: Fix sla checks being skipped when the preceding task instance misses the sla

Posted by GitBox <gi...@apache.org>.
doiken commented on PR #26968:
URL: https://github.com/apache/airflow/pull/26968#issuecomment-1297807728

   I fixed the conflict.
   Please take another look.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ephraimbuddy commented on a diff in pull request #26968: Fix sla checks being skipped when the preceding task instance misses the sla

Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on code in PR #26968:
URL: https://github.com/apache/airflow/pull/26968#discussion_r1012932975


##########
airflow/dag_processing/processor.py:
##########
@@ -425,7 +425,7 @@ def manage_slas(self, dag: DAG, session: Session = None) -> None:
                 if next_info is None:
                     break
                 if (ti.dag_id, ti.task_id, next_info.logical_date) in recorded_slas_query:
-                    break
+                    continue

Review Comment:
   This seems like a change in behaviour. It seems like it's going to record too many SLAs this way. 
   cc: @ashb 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] potiuk commented on pull request #26968: Fix sla checks being skipped when the preceding task instance misses the sla

Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #26968:
URL: https://github.com/apache/airflow/pull/26968#issuecomment-1350654737

   Just remind us about the PR - exactly what you just did. 
   
   And you can learn why if you watch the presentation:  https://airflowsummit.org/sessions/2022/hey-maintainer-exercise-your-empathy/ 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org