SameerMesiah97 commented on code in PR #61980:
URL: https://github.com/apache/airflow/pull/61980#discussion_r2814277425
##########
providers/dbt/cloud/tests/unit/dbt/cloud/triggers/test_dbt.py:
##########
@@ -219,7 +219,35 @@ async def test_dbt_job_run_timeout(self,
mock_get_job_status, mocked_is_still_ru
{
"status": "error",
"message": f"Job run {self.RUN_ID} has not reached a terminal
status "
- f"after {end_time} seconds.",
+ f"within the configured timeout.",
+ "run_id": self.RUN_ID,
+ }
+ )
+ assert expected == actual
+
+ @pytest.mark.asyncio
+
@mock.patch("airflow.providers.dbt.cloud.triggers.dbt.DbtCloudRunJobTrigger.is_still_running")
+
@mock.patch("airflow.providers.dbt.cloud.hooks.dbt.DbtCloudHook.get_job_status")
+ async def test_dbt_job_run_timeout_but_job_completes(
+ self, mock_get_job_status, mocked_is_still_running
+ ):
+ """Assert that a job completing at the timeout boundary is treated as
success, not timeout."""
Review Comment:
Nit: the docstring mentions "completing at the timeout boundary", but the
test appears to simulate completion after the deadline has technically passed
but before timeout emission. I would suggest clarifying it accordingly.
##########
providers/dbt/cloud/src/airflow/providers/dbt/cloud/triggers/dbt.py:
##########
@@ -75,17 +75,21 @@ async def run(self) -> AsyncIterator[TriggerEvent]:
hook = DbtCloudHook(self.conn_id, **self.hook_params)
try:
while await self.is_still_running(hook):
- if self.end_time < time.time():
- yield TriggerEvent(
- {
- "status": "error",
- "message": f"Job run {self.run_id} has not reached
a terminal status after "
- f"{self.end_time} seconds.",
- "run_id": self.run_id,
- }
- )
- return
await asyncio.sleep(self.poll_interval)
+ if self.end_time < time.time():
+ # Final status check: the job may have completed during
the sleep.
+ if await self.is_still_running(hook):
+ yield TriggerEvent(
+ {
+ "status": "error",
+ "message": f"Job run {self.run_id} has not
reached a terminal "
+ f"status within the configured timeout.",
+ "run_id": self.run_id,
+ }
+ )
+ return
+ # Job reached a terminal state — exit loop to handle below.
+ break
Review Comment:
It looks like the timeout emission could be extended by `poll_interval`,
meaning the actual timeout could occur up to one full polling cycle later than
the configured value. For example, with `timeout=60s` and `poll_interval=30s`,
the timeout may not be emitted until around 90s depending on scheduling.
I believe this behavior was already present prior to this PR, so it’s not a
regression. That said, we might consider sleeping for `min(poll_interval,
remaining_time)` to align the timeout more closely with the configured value.
This should not block the PR, but I think it might be worth exploring.
--
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]
