bingqin2 commented on issue #72982: URL: https://github.com/apache/airflow/issues/72982#issuecomment-5651090632
Hi @AbarnaaSree, please go ahead, it is yours. A few pointers from what I checked while writing this up: - `AzureBatchHook.__init__` only takes `azure_batch_conn_id`, and `get_conn()` builds `BatchClient(endpoint=..., credential=...)`. `azure-batch` 15 sits on `azure-core`, whose retry policy reads `retry_total` (and `retry_backoff_factor`, `retry_backoff_max`, `retry_status`) from the client kwargs, so `BatchClient(..., retry_total=n)` is enough; I verified the pipeline's retry policy picks it up. The operator builds the hook in its cached `hook` property, so the natural path is a new optional hook argument that the operator fills from `batch_max_retries`. - One decision to make up front: the operator's documented default is 3, but azure-core's default `retry_total` is 10. Passing the value through unconditionally would change behaviour for every existing user (10 retries become 3). Defaulting both the operator parameter and the new hook argument to `None` (meaning "SDK default") keeps behaviour unchanged unless someone sets the parameter. I would lean to that, but a maintainer may prefer honouring the documented 3, so it is worth stating the choice in the PR description. - For tests, patching `BatchClient` in the hook module and asserting the constructor kwargs is enough; no live client needed. Airflow does not assign issues: just open the PR, mention this issue in it, and I am happy to review. -- 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]
