Add configurable request timeout for the Airbyte API - #71092
Add configurable request timeout for the Airbyte API#71092filipeaaoliveira wants to merge 2 commits into
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
|
Hello! This is a gentle ping on this @potiuk I checked the last Airbyte provider PR and saw that you were the reviewer, hence why I am pinging you. Thank you |
potiuk
left a comment
There was a problem hiding this comment.
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:
# 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:
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— theexceptsetstimeout_ms = 0so the<= 0check below raises for both parse failures and non-positive values. It works, but reads as a trick; raising theValueErrordirectly in theexceptand 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.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Since apache-airflow-providers-airbyte 6.0.0 the provider uses airbyte-api 1.x, which switched from requests to httpx and applies a 5-second default request timeout. On self-hosted Airbyte deployments job creation can take far longer than that under load, so every trigger fails - and because the create-job call is not idempotent, the timed-out request still creates a job server-side, causing 409 errors and duplicate syncs on retry. Allow users to raise the timeout via a hook parameter or a connection extra.
Review on apache#71092 pointed out that the SDK's client-credentials hook sends the token request directly through the HTTP client, bypassing the SDK-level timeout_ms, so authenticated deployments could still hit the 5-second default on the token call. Setting the timeout on the httpx client covers the token request and all API operations alike, so the timeout_ms conversion is dropped. The client we build also follows redirects, matching the default client the SDK creates when none is supplied.
c7d7cf8 to
e13acdc
Compare
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. I hope this is better now, but if you have any more suggestions, please let me know. |
Hello everyone.
While upgrading Airflow to 3.3.0 yesterday, our Airbyte DAGs started failing on every trigger even though the syncs themselves were starting in Airbyte.
The cause is the provider's move to airbyte-api 1.x, which replaced requests with httpx and with it picked up httpx's 5-second default request timeout. Provider 6.0.0 requires the 1.x SDK, but 5.5.1 hits it too: its airbyte-api>=0.52.0 pin has no upper bound (#69081 added the <1.0.0 cap after 5.5.1 shipped), and constraints-3.3.0 resolves it to airbyte-api==1.0.1.
On self-hosted deployments, job creation can take far longer than 5 seconds under load — we measured POST /v1/jobs at 30–130 seconds with 9 connections triggered in parallel. Every trigger task then fails with
httpx.ReadTimeoutraised fromAirbyteHook.submit_sync_connection. And because create-job is not idempotent, the timed-out request still creates the job server-side, so retries get 409 "A sync is already running" and the attempt that eventually succeeds starts a duplicate sync.The hook currently exposes no way to change the timeout. This PR lets users set it through an
AirbyteHookparameter or a connection extra, passing it to the SDK's existingtimeout_ms. With neither set, the session is built exactly as before, so default behavior is unchanged.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code - Fable 5 following the guidelines