henry3260 commented on code in PR #67382:
URL: https://github.com/apache/airflow/pull/67382#discussion_r3294138495
##########
providers/airbyte/src/airflow/providers/airbyte/operators/airbyte.py:
##########
@@ -111,7 +111,7 @@ def execute(self, context: Context) -> None:
self.log.debug("Running in deferrable mode in job state %s...",
state)
if state in (JobStatusEnum.RUNNING, JobStatusEnum.PENDING,
JobStatusEnum.INCOMPLETE):
self.defer(
- timeout=self.execution_timeout,
Review Comment:
I think there is an edge case here. If `execution_timeout=30s`, but the
Airbyte job reaches `SUCCEEDED` after 45s, the trigger can still mark the task
as successful because it checks the Airbyte job status before checking
`execution_deadline`:
https://github.com/apache/airflow/blob/16ad4794f5a6c70480de7b137561f7f1bcdc2735/providers/airbyte/src/airflow/providers/airbyte/triggers/airbyte.py#L83-L102
After this PR changes `defer(timeout=None)`, the framework-level deferred
timeout no longer catches this case. Should the trigger check
`execution_deadline` before accepting `SUCCEEDED` / `CANCELLED`, so
`execution_timeout` remains a hard task-level limit?
--
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]
