argibbs commented on code in PR #29933:
URL: https://github.com/apache/airflow/pull/29933#discussion_r1126159457
##########
airflow/sensors/external_task.py:
##########
@@ -273,6 +284,31 @@ def poke(self, context, session=None):
)
raise AirflowException(f"The external DAG
{self.external_dag_id} failed.")
+ count_skipped = -1
+ if self.skipped_states:
+ count_skipped = self.get_count(dttm_filter, session,
self.skipped_states)
+
+ # Skip if anything in the list has skipped. Note if we are checking
multiple tasks and one skips
+ # before another errors, we'll skip first.
Review Comment:
This initial change is simply fixing the problem I have where I only have a
single monitored task (or dag). As you're noting, Multiple tasks complicates
things - I did give this some thought hence the comment in the code...
> Maybe we can improve the sensor by adding something similar to TriggerRule
Can you expand with an explicit example? Given that this change is entirely
additive in terms of behaviour, I'm concerned about scope creep. Or to put it
another way, YAGNI. This PR solves a problem I actually have. I don't want to
go and complicate the change without an actual use case, which realistically
isn't going to come from me (yet!). If this makes it into the main branch, and
people start using it and then ask for more levers on the behaviour (e.g.
something like a min-skip-count threshold), I'd want to make the change then.
> or just skip it when nothing is fail and we have at least one task in
skipped state.
I believe this is what the code does? If anything's failed, we'll raise an
AirflowException and fail. If not, we then check for skips, and if anything's
skipped we'll raise an AirflowSkipException. (And then obviously we check if
everything's in a good state, and go good if so)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
