seanxwzhang commented on pull request #8545:
URL: https://github.com/apache/airflow/pull/8545#issuecomment-626397934
> Option 1 doesn't guarantee correctness right? i.e. if there are more
dagruns that need to be checked than the preset limit, some of them will be
ignored?
True. I guess the way to do it (if no addition column is added) would be to
remove the the fixed count, and simply do
```
scheduled_dagruns = (
session.query(DR)
.filter(DR.dag_id == self.dag_id)
.filter(DR.run_id.notlike(f"{DagRunType.BACKFILL_JOB.value}__%"))
.filter(DR.external_trigger == False)
.filter(DR.state != State.SUCCESS)
.order_by(desc(DR.execution_date))
.all()
)
```
This way we only get DR that have yet to succeed (since we made an
assumption that successful DRs are free from SLA check).
> With regards to performance comparison between option 1 and option 2,
aren't we already checking all the TIs for the 100 fetched dag runs in option 1?
We are checking whether these TIs **are violating SLAs**, not whether these
TIs **are free from SLAs**, those are different checks (e.g., to check if a TI
violates *expected_duration*, we compare the current duration with the SLA; to
check if a TI is free from SLA violations, we assert on that the TI has
finished within the *expected_duration*). To do so would require us adding
another column to TI as well.
I'm slightly inclined towards option 1 (probably need to remove the 100
fixed limit), but definitely open to other opinions. :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
