shahar1 commented on code in PR #74602:
URL: https://github.com/apache/airflow/pull/74602#discussion_r4502966170
##########
providers/google/src/airflow/providers/google/cloud/hooks/dataproc.py:
##########
@@ -1985,6 +1986,12 @@ async def delete_batch(
)
@GoogleBaseHook.fallback_to_default_project_id
+ @tenacity_retry(
+ retry=retry_if_exception_type(ServiceUnavailable),
Review Comment:
minor — retry scope. The sync `wait_for_batch` in this same hook retries on
any `ServerError` (500/502/503/504), while this retries `ServiceUnavailable`
only. `ServerError` is already imported above; using it keeps the deferred path
consistent with the sync one for the other transient 5xx responses (the
`ServiceUnavailable` import then goes away).
```suggestion
retry=retry_if_exception_type(ServerError),
```
##########
providers/google/src/airflow/providers/google/cloud/hooks/dataproc.py:
##########
@@ -1985,6 +1986,12 @@ async def delete_batch(
)
@GoogleBaseHook.fallback_to_default_project_id
+ @tenacity_retry(
Review Comment:
nit — retries are silent. The sync `wait_for_batch` logs each retry
("Retrying. Dataproc API returned server error…"), and `operators/gcs.py`
passes `before_sleep=tenacity.before_sleep_log(..., logging.WARNING)` to its
tenacity retries. A module-level `logging.getLogger(__name__)` passed to
`before_sleep_log` would make a retried 503 visible in the triggerer log.
Optional.
##########
providers/google/tests/unit/google/cloud/hooks/test_dataproc.py:
##########
@@ -1074,6 +1075,33 @@ async def test_get_batch(self, mock_client):
timeout=None,
)
+ @pytest.mark.asyncio
+ @mock.patch("asyncio.sleep", new_callable=AsyncMock)
+ @mock.patch(DATAPROC_STRING.format("DataprocAsyncHook.get_batch_client"))
+ async def test_get_batch_retries_on_service_unavailable(self, mock_client,
mock_sleep):
+ mock_batch_client = AsyncMock(BatchControllerAsyncClient)
+ mock_client.return_value = mock_batch_client
+ expected_batch = mock.MagicMock()
Review Comment:
minor — unspec'd mock. Review checklist: *"Flag any `mock.Mock()` or
`mock.MagicMock()` without `spec` or `autospec`. Unspec'd mocks silently accept
any attribute access, hiding real bugs."* `Batch` is already imported in this
module.
```suggestion
expected_batch = mock.MagicMock(spec=Batch)
```
--
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]
