filipeaaoliveira commented on PR #71092: URL: https://github.com/apache/airflow/pull/71092#issuecomment-5271010958
> Thanks for this — the write-up is unusually good: the diagnosis (airbyte-api 1.x moving to httpx and inheriting its 5s default), the non-idempotent create-job making retries produce duplicate syncs, and the constraint resolution that drags 5.5.1 into the same hole are all accurate. I checked the plumbing against `airbyte-api` 1.x and it does what you describe: `timeout_ms` lands in `sdk_configuration.timeout_ms`, each operation falls back to it, and `build_request()` uses `httpx.USE_CLIENT_DEFAULT` when it is `None` — so the "unchanged when unset" claim holds, and it works with the proxy-mounted client too. > > One gap that matters for your own use case, and a small style point. > > ### The OAuth token request is not covered by the timeout > `timeout_ms` only applies to API operations. The SDK's client-credentials hook fetches the access token itself: > > ```python > # airbyte_api/_hooks/clientcredentials.py > response = self.client.send( > self.client.build_request(method="POST", url=token_url, data=payload) > ) > ``` > > No timeout override there, so that request keeps the httpx client default of 5 seconds regardless of what is configured. On exactly the loaded deployment this PR targets, `submit_sync_connection` can still fail with `httpx.ReadTimeout` — just on the token call rather than on `POST /v1/jobs` — and you are back to the duplicate-sync problem. > > Setting the timeout on the httpx client covers both paths, and drops the millisecond conversion entirely: > > ```python > if self.conn["proxies"] or timeout is not None: > client = httpx.Client(mounts=mounts, timeout=timeout if timeout is not None else 5.0) > ``` > > Keeping `timeout_ms` as well is harmless if you prefer belt-and-braces, but the client-level timeout is the part that makes the token fetch safe. > > ### Smaller observations > * `hooks/airbyte.py:139-141` — the `except` sets `timeout_ms = 0` so the `<= 0` check below raises for both parse failures and non-positive values. It works, but reads as a trick; raising the `ValueError` directly in the `except` and keeping the range check separate says the same thing more plainly. > * The timeout is reachable only from the hook constructor or the connection extra, not from `AirbyteTriggerSyncOperator`. The extra covers the common deployment-wide case and the docs say so, so this is fine as-is — flagging only in case you want the operator to take it too. > > The rest looks right: `ValueError` rather than `AirflowException`, docs updated alongside the code, no newsfragment (correct — provider changelogs are regenerated from git log), and the tests parametrize both the accepted and the rejected shapes. > > > _This review was drafted by an AI-assisted tool and > > confirmed by an Airflow maintainer. The findings > > below are observations, not blockers; an Airflow > > maintainer — a real person — will take the next look at the > > PR. If you think a finding is mis-applied, please reply on > > the PR and a maintainer will weigh in._ > > _More on how Airflow handles maintainer review:_ > > [contributing-docs/05_pull_requests.rst](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst). > > Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting Hello @potiuk ! Thank you so much for the review. You're completely right on both things and I've updated them like you recommended. Thank you once again for taking the time to review and make suggestions. It looks and it IS way better this way. One thing that I added was that on the client, it now passes `follow_redirects=True` to match the default client the SDK creates when none is supplied. I hope this is better now, but if you have any more suggestions, please let me know. -- 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]
