shahar1 commented on code in PR #34785:
URL: https://github.com/apache/airflow/pull/34785#discussion_r1367934383
##########
airflow/providers/google/cloud/hooks/dataflow.py:
##########
@@ -420,7 +420,9 @@ def _check_dataflow_job_state(self, job) -> bool:
"JOB_STATE_DRAINED while it is a batch job"
)
- if not self._wait_until_finished and current_state ==
self._expected_terminal_state:
+ if current_state == self._expected_terminal_state:
+ if self._expected_terminal_state ==
DataflowJobStatus.JOB_STATE_RUNNING:
+ return not self._wait_until_finished
Review Comment:
I'm the author of #34217 and sincerely apologize for the mess!
I ensured that the existing tests passed, but I didn't consider adding the
`JOB_STATE_DONE` - which is problematic for that `if` case.
As @champon1020 stated, `_wait_until_finished` should have been applied
preferentially compared to `expected_terminal_state` to avoid breaking default
behavior.
I agree that having both params is somewhat confusing, and we should revise
deprecating `_wait_until_finished` in the future.
The suggested solution LGTM, thank you for fixing it up!
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
