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/03/12 18:05:49 UTC
[GitHub] [airflow] dstandish opened a new pull request #22214: Make SlaMiss test stricter
dstandish opened a new pull request #22214:
URL: https://github.com/apache/airflow/pull/22214
Tests previously did not document that the SlaMiss records created are for TIs that follow completed TIs. Importantly, the TIs for which SLA misses are created may not actually exist in the DB, and this is an important thing to document e.g. if one considers adding a foreign key to TI.
--
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] ashb commented on a change in pull request #22214: Make SlaMiss test stricter
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #22214:
URL: https://github.com/apache/airflow/pull/22214#discussion_r825337571
##########
File path: tests/dag_processing/test_processor.py
##########
@@ -222,15 +222,47 @@ def test_dag_file_processor_sla_miss_doesnot_raise_integrity_error(self, dag_mak
dag_file_processor = DagFileProcessor(dag_ids=[], log=mock.MagicMock())
dag_file_processor.manage_slas(dag=dag, session=session)
- sla_miss_count = (
+ sla_misses = (
session.query(SlaMiss)
.filter(
SlaMiss.dag_id == dag.dag_id,
SlaMiss.task_id == task.task_id,
)
- .count()
+ .all()
)
- assert sla_miss_count == 1
+ assert len(sla_misses) == 1
+ created_sla_miss = sla_misses[0]
+
+ # the SlaMiss created is for the _next_ TI
+ assert created_sla_miss.execution_date == test_start_date + datetime.timedelta(days=1)
Review comment:
SLA is defined as
```
:param sla: time by which the job is expected to succeed. Note that
this represents the ``timedelta`` after the period is closed. For
example if you set an SLA of 1 hour, the scheduler would send an email
soon after 1:00AM on the ``2016-01-02`` if the ``2016-01-01`` instance
has not succeeded yet.
The scheduler pays special attention for jobs with an SLA and
sends alert
emails for SLA misses. SLA misses are also recorded in the database
for future reference. All tasks that share the same SLA time
get bundled in a single email, sent soon after that time. SLA
notification are sent once and only once for each task instance.
```
So I don't see wht the dttm is for the wrong TI.
--
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] ashb commented on a change in pull request #22214: Make SlaMiss test stricter
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #22214:
URL: https://github.com/apache/airflow/pull/22214#discussion_r825336536
##########
File path: tests/dag_processing/test_processor.py
##########
@@ -222,15 +222,47 @@ def test_dag_file_processor_sla_miss_doesnot_raise_integrity_error(self, dag_mak
dag_file_processor = DagFileProcessor(dag_ids=[], log=mock.MagicMock())
dag_file_processor.manage_slas(dag=dag, session=session)
- sla_miss_count = (
+ sla_misses = (
session.query(SlaMiss)
.filter(
SlaMiss.dag_id == dag.dag_id,
SlaMiss.task_id == task.task_id,
)
- .count()
+ .all()
)
- assert sla_miss_count == 1
+ assert len(sla_misses) == 1
+ created_sla_miss = sla_misses[0]
+
+ # the SlaMiss created is for the _next_ TI
+ assert created_sla_miss.execution_date == test_start_date + datetime.timedelta(days=1)
Review comment:
This is the current behaviour, but that seems.... wrong? How can a task that doesn't exist have breached it's SLA? (I guess if the TI isn't created)
But I would have assumes that the execution date of the SLA miss should have been the one for the TI that does exist in this test 🤔
--
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] ashb commented on a change in pull request #22214: Make SlaMiss test stricter
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #22214:
URL: https://github.com/apache/airflow/pull/22214#discussion_r825337610
##########
File path: tests/dag_processing/test_processor.py
##########
@@ -222,15 +222,47 @@ def test_dag_file_processor_sla_miss_doesnot_raise_integrity_error(self, dag_mak
dag_file_processor = DagFileProcessor(dag_ids=[], log=mock.MagicMock())
dag_file_processor.manage_slas(dag=dag, session=session)
- sla_miss_count = (
+ sla_misses = (
session.query(SlaMiss)
.filter(
SlaMiss.dag_id == dag.dag_id,
SlaMiss.task_id == task.task_id,
)
- .count()
+ .all()
)
- assert sla_miss_count == 1
+ assert len(sla_misses) == 1
+ created_sla_miss = sla_misses[0]
+
+ # the SlaMiss created is for the _next_ TI
+ assert created_sla_miss.execution_date == test_start_date + datetime.timedelta(days=1)
Review comment:
It looks like it's been like this _for ever_ https://github.com/apache/airflow/commit/60f35c3aeca6206233eec8c3371c797824a9f2f8
But then there's also been lots of reports that SLA is not working right, and maybe this might be part of why?
--
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