You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "amoghrajesh (via GitHub)" <gi...@apache.org> on 2025/10/17 11:03:43 UTC
[PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
amoghrajesh opened a new pull request, #56762:
URL: https://github.com/apache/airflow/pull/56762
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
## Motivation
The task SDK uses httpx for HTTP operations but depends on `retryhttp` for retry logic. This brings in the entire `requests` library as a transitive dependency, even though it's never actually used.
Memray reports also show some stats which can be improved.
For `import retryhttp`:
<img width="2557" height="883" alt="image" src="https://github.com/user-attachments/assets/e7f7f1d3-c9a8-46db-9052-6deb626cab32" />
<img width="1141" height="624" alt="image" src="https://github.com/user-attachments/assets/1ddad4b9-790c-4020-a41c-425b966ab3b1" />
<img width="1141" height="624" alt="image" src="https://github.com/user-attachments/assets/df6d5f18-6871-4314-ab92-ca9c5a7eb483" />
This is because retryhttp unconditionally imports both httpx and requests, even though we only use httpx.
The retry handler works fine, but it's unnecessary bloat. We're already using tenacity in sdk client, and we only use httpx, not requests, so might as well use tenacity for better memory results and reduced footprint.
## Alternatives Considered
### 1. Switch to stamina
- Modern, opinionated wrapper around tenacity
- Better ergonomics and async support
### 2. Use pure tenacity
- Zero new dependencies (tenacity already used)
- Maximum memory and size savings
- Full control over retry behavior
Went ahead and selected tenacity because it is a battle tested library already present in task sdk and is used often and there is no need for a new library when the benefits of memory footprint (Net savings: ~444KB vs ~544KB with pure tenacity) aren't significantly high.
## Changes of note
Migrated task SDK to use tenacity instead of retryhttp while maintaining total parity with what retryhttp offered. A lot of code thats written is inspired by code of retryhttp!
Tenacity is a generic retry library - it doesn't know anything about HTTP. It just retries when you tell it to, so to maintain parity, some wrappers and helpers had to be written.
**`_should_retry_api_request(exception)`**
This function determines which errors should trigger a retry. It replicates retryhttp's behavior of retrying on:
- Server errors (5xx status codes)
- Rate limiting (429 status code)
- Network failures (httpx.NetworkError)
- Timeouts (httpx.TimeoutException)
But NOT retrying on client errors like 404 or 401, which would be pointless.
**`_get_retry_wait_time(retry_state)`**
This function handles the wait time between retries. It replicates retryhttp's `wait_retry_after` behavior:
- For 429 responses with a `Retry-After` header, it uses that exact value
- Otherwise, it falls back to exponential backoff with random jitters
### How parity was maintained
The original retryhttp decorator looked like this:
```python
@retry(
max_attempt_number=API_RETRIES,
wait_server_errors=_default_wait,
wait_network_errors=_default_wait,
wait_timeouts=_default_wait,
wait_rate_limited=wait_retry_after(fallback=_default_wait),
before_sleep=before_log(log, logging.WARNING),
)
```
Each of those parameters mapped to specific HTTP behaviors. Our new decorator with tenacity:
```python
@retry(
retry=retry_if_exception(_should_retry_api_request),
stop=stop_after_attempt(API_RETRIES),
wait=_get_retry_wait_time,
before_sleep=before_log(log, logging.WARNING),
reraise=True,
)
```
Removed from dependencies:
- `retryhttp`
- `requests`
- `types-requests`
## How this was tested
### Unit tests
All existing retry tests pass without modification:
- Server error recovery (500 errors with retry)
- Rate limiting with Retry-After header (429 responses)
- Non-retryable errors (422 validation errors)
- Max retry attempts exhaustion
### Integration testing
Ran the task SDK against a live API server and killed the server mid request. The client correctly retried with exponential backoff and recovered when the server came back up, confirming network error handling works as expected.
<img width="1665" height="152" alt="image" src="https://github.com/user-attachments/assets/8a78fd22-ecfc-4c39-829a-eba0c62dfe65" />
## Benefits
### Memray results
Importing `import tenacity` and showing difference:
Flamegraph (now):
<img width="2559" height="833" alt="image" src="https://github.com/user-attachments/assets/351e231b-53e2-4817-95f4-5092b800e3e6" />
Flamegraph (earlier):
<img width="2557" height="883" alt="image" src="https://github.com/user-attachments/assets/e7f7f1d3-c9a8-46db-9052-6deb626cab32" />
Memory Graph (now):
<img width="1142" height="631" alt="image" src="https://github.com/user-attachments/assets/b5c0d18d-f5d4-4337-ac22-2d3d881db1bc" />
Memory Graph (before):
<img width="1141" height="624" alt="image" src="https://github.com/user-attachments/assets/1ddad4b9-790c-4020-a41c-425b966ab3b1" />
Stats (now):
<img width="1142" height="631" alt="image" src="https://github.com/user-attachments/assets/9bf0afc1-d17f-4557-839c-97bd0295dc3b" />
Stats (earlier):
<img width="1141" height="624" alt="image" src="https://github.com/user-attachments/assets/df6d5f18-6871-4314-ab92-ca9c5a7eb483" />
### Package size
INSTALLED DEPENDENCIES SIZE:
Removed packages:
• retryhttp ..................... 40K
• requests ..................... 228K
• urllib3 (dep of requests) .... 488K
• charset_normalizer (dep) ..... 808K
• idna (dep) ................... 352K
• certifi (dep) ................ 296K
─────────────────────────────────────
TOTAL DISK SAVINGS: ........... ~2.2 MB
Workers do not need to have these dependencies anymore.
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2452111586
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,14 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retried based on the exception type."""
Review Comment:
> Consider adding a brief comment in _should_retry_api_request about why 429s aren’t retried anymore.
No need, it never worked :), that comment would only mislead.
> Nits: Update PR description to match the final behaviour (since 429 handling was dropped).
Thanks, that has been updated now.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2439447365
##########
task-sdk/tests/task_sdk/api/test_client.py:
##########
@@ -225,6 +225,42 @@ def test_retry_handling_ok(self, mock_sleep):
assert len(responses) == 1
assert mock_sleep.call_count == 0
+ @mock.patch("time.sleep", return_value=None)
Review Comment:
I'd suggest using timemachine instead of mocking time.sleep.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3415870154
@amoghrajesh It might be worth adding `import pydantic, httpx` in to those benchmarks, since we really only care about the delta on top of those modules, not the standalone.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2454168668
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,14 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retried based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ return exception.response.status_code >= 500
Review Comment:
Ran a simple script for checking difference:
Testing _should_retry_api_request:
HTTPStatusError: True (expected True)
HTTPStatusError: True (expected True)
HTTPStatusError: True (expected True)
HTTPStatusError: False (expected False)
HTTPStatusError: False (expected False)
HTTPStatusError: False (expected False)
HTTPStatusError: False (expected False)
ConnectError: True (expected True)
ReadTimeout: True (expected True)
WriteError: True (expected True)
ProxyError: True (expected True)
ValueError: False (expected False)
RuntimeError: False (expected False)
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2439432206
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,37 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retrie based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ status = exception.response.status_code
+ return status >= 500 or status == 429
+
+ # for all other httpx errors (network, timeout, connect, etc.), retry
+ if isinstance(exception, httpx.RequestError):
+ return True
+
+ return False
+
+
+def _get_retry_wait_time(retry_state) -> float:
+ """
+ Calculate wait time for retry, respecting Retry-After header on 429 responses.
+
+ For rate limit responses (429) with a Retry-After header, uses the value from the header.
+ Otherwise, fall bacsk to exponential backoff with jitter.
+ """
+ exception = retry_state.outcome.exception()
+
+ if isinstance(exception, httpx.HTTPStatusError):
+ if exception.response.status_code == 429:
+ retry_after = exception.response.headers.get("Retry-After")
+ if retry_after and retry_after.isdigit():
+ return float(retry_after)
+
+ return wait_random_exponential(min=API_RETRY_WAIT_MIN, max=API_RETRY_WAIT_MAX)(retry_state)
Review Comment:
What does the diff look like with Stamina? The memory difference is negligible, so if the code is nicer as a result maybe it's worth using that
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3430828820
## Test Setup
1. Testing baseline
```python
"""Test baseline: pydantic + httpx only"""
import pydantic
import httpx
from pydantic import BaseModel
class TestModel(BaseModel):
name: str
model = TestModel(name="test")
client = httpx.Client()
client.close()
print("Baseline: pydantic + httpx loaded")
```
2. Testing with tenacity
```python
"""Test tenacity with equivalent retry configuration"""
import pydantic
import httpx
import tenacity
from pydantic import BaseModel
from tenacity import retry, retry_if_exception, stop_after_attempt, wait_exponential
class TestModel(BaseModel):
name: str
model = TestModel(name="test")
client = httpx.Client()
client.close()
# Equivalent retry configuration
@retry(
retry=retry_if_exception(lambda e: isinstance(e, Exception)),
stop=stop_after_attempt(3),
wait=wait_exponential(multiplier=1, min=0.1, max=1.0),
reraise=True,
)
def dummy():
pass
print("Tenacity with equivalent config loaded")
```
3. Testing with stamina
```python
"""Test stamina with equivalent retry configuration"""
import pydantic
import httpx
import stamina
from pydantic import BaseModel
class TestModel(BaseModel):
name: str
model = TestModel(name="test")
client = httpx.Client()
client.close()
# Equivalent retry configuration
@stamina.retry(
on=Exception,
attempts=3,
wait_initial=0.1,
wait_max=1.0,
wait_jitter=1.0,
)
def dummy():
pass
print("Stamina with equivalent config loaded")
```
4. Testing with retryhttp
```python
"""Test retryhttp with equivalent retry configuration"""
import pydantic
import httpx
import retryhttp
from pydantic import BaseModel
from retryhttp import retry
class TestModel(BaseModel):
name: str
model = TestModel(name="test")
client = httpx.Client()
client.close()
# Equivalent retry configuration (retryhttp uses different API)
@retry(
max_attempt_number=3,
# retryhttp uses different wait strategies - using default exponential
)
def dummy():
pass
print("RetryHTTP with equivalent config loaded")
```
### Run profiling
```bash
memray run --output baseline.bin test_baseline.py
memray run --output tenacity.bin test_tenacity.py
memray run --output stamina.bin test_stamina.py
memray run --output retryhttp.bin test_retryhttp.py
memray summary baseline.bin
memray summary tenacity.bin
memray summary stamina.bin
memray summary retryhttp.bin
```
### Benchmark Results
| Library | Total Memory | Overhead | Savings vs RetryHTTP | % Reduction |
|---------|-------------|----------|---------------------|-------------|
| **Baseline** (pydantic + httpx) | 13.62 MB | - | - | - |
| **Tenacity** (equivalent config) | 17.03 MB | +3.41 MB | **4.22 MB** | **50%** |
| **Stamina** (equivalent config) | 17.65 MB | +4.03 MB | **3.60 MB** | **42%** |
| **RetryHTTP** (equivalent config) | 21.25 MB | +7.63 MB | - | - |
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2440058134
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,37 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retrie based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ status = exception.response.status_code
+ return status >= 500 or status == 429
+
+ # for all other httpx errors (network, timeout, connect, etc.), retry
+ if isinstance(exception, httpx.RequestError):
+ return True
+
+ return False
+
+
+def _get_retry_wait_time(retry_state) -> float:
+ """
+ Calculate wait time for retry, respecting Retry-After header on 429 responses.
+
+ For rate limit responses (429) with a Retry-After header, uses the value from the header.
+ Otherwise, fall bacsk to exponential backoff with jitter.
+ """
+ exception = retry_state.outcome.exception()
+
+ if isinstance(exception, httpx.HTTPStatusError):
+ if exception.response.status_code == 429:
+ retry_after = exception.response.headers.get("Retry-After")
+ if retry_after and retry_after.isdigit():
+ return float(retry_after)
+
+ return wait_random_exponential(min=API_RETRY_WAIT_MIN, max=API_RETRY_WAIT_MAX)(retry_state)
Review Comment:
Where did that `Retry-After` header come from though?
I do not recall API-server (via FastAPI) setting that header.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh merged PR #56762:
URL: https://github.com/apache/airflow/pull/56762
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3415336232
Created a PR on their repo https://github.com/austind/retryhttp/pull/26 to fix that
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2451199720
##########
task-sdk/tests/task_sdk/api/test_client.py:
##########
@@ -225,6 +225,42 @@ def test_retry_handling_ok(self, mock_sleep):
assert len(responses) == 1
assert mock_sleep.call_count == 0
+ @mock.patch("time.sleep", return_value=None)
+ def test_retry_handling_network_error(self, mock_sleep):
+ """Test that network errors trigger retry and eventually recover."""
+ call_count = 0
+
+ def handle_request(request: httpx.Request) -> httpx.Response:
+ nonlocal call_count
+ call_count += 1
+ if call_count < 3:
+ raise httpx.NetworkError("Connection failed")
+ return httpx.Response(200, json={"detail": "Recovered from error"})
+
+ client = make_client(transport=httpx.MockTransport(handle_request))
+ response = client.get("http://error")
+ assert response.status_code == 200
+ assert call_count == 3
+ assert mock_sleep.call_count == 2
+
+ @mock.patch("time.sleep", return_value=None)
Review Comment:
Made changes to the entire file
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3432371355
> LGTM overall — clear improvement and simpler deps.
>
> I did not understand completely why we removed 429 retry logic. Earlier (per the PR description and early commits), the Task SDK’s retry logic used to respect Retry-After headers from 429 responses - pausing before retrying.
>
> In the final version, the `_should_retry_api_request` predicate only retries on:
>
> * `httpx.RequestError` (network / timeout)
> * `httpx.HTTPStatusError` with status ≥ 500
>
> It no longer retries on 429, nor does it read Retry-After. Why?
Thanks @sunank200, the reason for that is simple -- API server doesn't have feature support for rate limiting! So it doesnt make sense to have the client support it when server doesn't: https://github.com/apache/airflow/pull/56762#discussion_r2440083758
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3415497276
Memray for stamina too: `from stamina import retry"
<img width="2560" height="667" alt="image" src="https://github.com/user-attachments/assets/e9cd1858-952a-4568-9e79-f73ae4cc3af5" />
<img width="1140" height="667" alt="image" src="https://github.com/user-attachments/assets/5969bd3d-6a12-44c0-806e-a1ec3b982048" />
<img width="1140" height="667" alt="image" src="https://github.com/user-attachments/assets/714d63e5-f787-49f7-8e88-6235a02846b9" />
Turns out it is even better than `tenacity`
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2451202879
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,37 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retrie based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ status = exception.response.status_code
+ return status >= 500 or status == 429
+
+ # for all other httpx errors (network, timeout, connect, etc.), retry
+ if isinstance(exception, httpx.RequestError):
+ return True
+
+ return False
+
+
+def _get_retry_wait_time(retry_state) -> float:
+ """
+ Calculate wait time for retry, respecting Retry-After header on 429 responses.
+
+ For rate limit responses (429) with a Retry-After header, uses the value from the header.
+ Otherwise, fall bacsk to exponential backoff with jitter.
+ """
+ exception = retry_state.outcome.exception()
+
+ if isinstance(exception, httpx.HTTPStatusError):
+ if exception.response.status_code == 429:
+ retry_after = exception.response.headers.get("Retry-After")
+ if retry_after and retry_after.isdigit():
+ return float(retry_after)
+
+ return wait_random_exponential(min=API_RETRY_WAIT_MIN, max=API_RETRY_WAIT_MAX)(retry_state)
Review Comment:
Removed in latest commits
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3431371360
@kaxil I have added this to 3.1.2 (we could also consider 3.2), but do you think there is a reason to add it to 3.1.1? I think not.
@jscheffl I see similar patterns for edge executor too, you could adopt a similar pattern there too. LMK if you want me to take a stab at it.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3415574725
> Turns out it is even better than `tenacity`
That can't be true though. Something must be wrong in how you are testing it. Stamina uses [tenacity](https://github.com/hynek/stamina/blob/main/pyproject.toml#L28) under the hood.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3430791665
> @amoghrajesh It might be worth adding `import pydantic, httpx` in to those benchmarks, since we really only care about the delta on top of those modules, not the standalone.
Good idea. Ran a test for that, posting results below.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "sunank200 (via GitHub)" <gi...@apache.org>.
sunank200 commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2451960118
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,14 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retried based on the exception type."""
Review Comment:
Consider adding a brief comment in _should_retry_api_request about why 429s aren’t retried anymore.
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,14 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retried based on the exception type."""
Review Comment:
Nits: Update PR description to match the final behaviour (since 429 handling was dropped).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2439981961
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,37 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retrie based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ status = exception.response.status_code
+ return status >= 500 or status == 429
+
+ # for all other httpx errors (network, timeout, connect, etc.), retry
+ if isinstance(exception, httpx.RequestError):
+ return True
+
+ return False
+
+
+def _get_retry_wait_time(retry_state) -> float:
+ """
+ Calculate wait time for retry, respecting Retry-After header on 429 responses.
+
+ For rate limit responses (429) with a Retry-After header, uses the value from the header.
+ Otherwise, fall bacsk to exponential backoff with jitter.
+ """
+ exception = retry_state.outcome.exception()
+
+ if isinstance(exception, httpx.HTTPStatusError):
+ if exception.response.status_code == 429:
+ retry_after = exception.response.headers.get("Retry-After")
+ if retry_after and retry_after.isdigit():
+ return float(retry_after)
+
+ return wait_random_exponential(min=API_RETRY_WAIT_MIN, max=API_RETRY_WAIT_MAX)(retry_state)
Review Comment:
From what I can understand about stamina, it does not allow using custom wait strategies (what we do for 429 status code above), it can be worked around but IDK how important it is (can we accept losing that?)
Anyways, the diff looks like such:
```diff
diff --git a/task-sdk/src/airflow/sdk/api/client.py b/task-sdk/src/airflow/sdk/api/client.py
--- a/task-sdk/src/airflow/sdk/api/client.py (revision 9b7d5100c658492e2f91711e8ffe9d1e45e43ca9)
+++ b/task-sdk/src/airflow/sdk/api/client.py (date 1760706304732)
@@ -30,13 +30,7 @@
import msgspec
import structlog
from pydantic import BaseModel
-from tenacity import (
- before_log,
- retry,
- retry_if_exception,
- stop_after_attempt,
- wait_random_exponential,
-)
+import stamina
from uuid6 import uuid7
from airflow.configuration import conf
@@ -830,24 +824,6 @@
return False
-def _get_retry_wait_time(retry_state) -> float:
- """
- Calculate wait time for retry, respecting Retry-After header on 429 responses.
-
- For rate limit responses (429) with a Retry-After header, uses the value from the header.
- Otherwise, fall bacsk to exponential backoff with jitter.
- """
- exception = retry_state.outcome.exception()
-
- if isinstance(exception, httpx.HTTPStatusError):
- if exception.response.status_code == 429:
- retry_after = exception.response.headers.get("Retry-After")
- if retry_after and retry_after.isdigit():
- return float(retry_after)
-
- return wait_random_exponential(min=API_RETRY_WAIT_MIN, max=API_RETRY_WAIT_MAX)(retry_state)
-
-
class Client(httpx.Client):
def __init__(self, *, base_url: str | None, dry_run: bool = False, token: str, **kwargs: Any):
if (not base_url) ^ dry_run:
@@ -881,12 +857,12 @@
log.debug("Execution API issued us a refreshed Task token")
self.auth = BearerAuth(new_token)
- @retry(
- retry=retry_if_exception(_should_retry_api_request),
- stop=stop_after_attempt(7),
- wait=_get_retry_wait_time,
- before_sleep=before_log(log, logging.WARNING),
- reraise=True,
+ @stamina.retry(
+ on=_should_retry_api_request,
+ attempts=API_RETRIES,
+ wait_initial=API_RETRY_WAIT_MIN,
+ wait_max=API_RETRY_WAIT_MAX,
+ wait_jitter=1.0,
)
def request(self, *args, **kwargs):
"""Implement a convenience for httpx.Client.request with a retry layer."""
```
So basically very similar diff, needs `_should_retry_api_request`
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3415614263
@kaxil yes you are right, something was wrong with my setup.
Testing using simple CLI now and tenacity stands better:
```bash
cd /Users/amoghdesai/Documents/OSS/repos/airflow
source .venv/bin/activate
cat > /tmp/test_retry_tenacity.py << 'EOF'
from tenacity import retry
print("Imported retry from tenacity")
EOF
cat > /tmp/test_retry_stamina.py << 'EOF'
from stamina import retry
print("Imported retry from stamina")
EOF
memray run -o retry-tenacity.bin /tmp/test_retry_tenacity.py
memray run -o retry-stamina.bin /tmp/test_retry_stamina.py
echo "=== TENACITY ==="
memray stats retry-tenacity.bin | head -20
echo ""
echo "=== STAMINA ==="
memray stats retry-stamina.bin | head -20
```
Results:
TENACITY: from tenacity import retry
Peak memory: 1.523 MB
Total allocated: 3.989 MB
Allocations: 1,343
STAMINA: from stamina import retry
Peak memory: 2.655 MB
Total allocated: 6.805 MB
Allocations: 2,139
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2440083758
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,37 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retrie based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ status = exception.response.status_code
+ return status >= 500 or status == 429
+
+ # for all other httpx errors (network, timeout, connect, etc.), retry
+ if isinstance(exception, httpx.RequestError):
+ return True
+
+ return False
+
+
+def _get_retry_wait_time(retry_state) -> float:
+ """
+ Calculate wait time for retry, respecting Retry-After header on 429 responses.
+
+ For rate limit responses (429) with a Retry-After header, uses the value from the header.
+ Otherwise, fall bacsk to exponential backoff with jitter.
+ """
+ exception = retry_state.outcome.exception()
+
+ if isinstance(exception, httpx.HTTPStatusError):
+ if exception.response.status_code == 429:
+ retry_after = exception.response.headers.get("Retry-After")
+ if retry_after and retry_after.isdigit():
+ return float(retry_after)
+
+ return wait_random_exponential(min=API_RETRY_WAIT_MIN, max=API_RETRY_WAIT_MAX)(retry_state)
Review Comment:
It doesn't, no.
The migration here is a direct port-over of what was done earlier via retryhttp. It had support for 429 even when API server doesn't support it as of today -- maybe some day? Not so sure if we want to keep support since its a no-op for client now.
`Retry-After` is often a common header sent for rate limit status codes: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/429
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2440083758
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,37 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retrie based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ status = exception.response.status_code
+ return status >= 500 or status == 429
+
+ # for all other httpx errors (network, timeout, connect, etc.), retry
+ if isinstance(exception, httpx.RequestError):
+ return True
+
+ return False
+
+
+def _get_retry_wait_time(retry_state) -> float:
+ """
+ Calculate wait time for retry, respecting Retry-After header on 429 responses.
+
+ For rate limit responses (429) with a Retry-After header, uses the value from the header.
+ Otherwise, fall bacsk to exponential backoff with jitter.
+ """
+ exception = retry_state.outcome.exception()
+
+ if isinstance(exception, httpx.HTTPStatusError):
+ if exception.response.status_code == 429:
+ retry_after = exception.response.headers.get("Retry-After")
+ if retry_after and retry_after.isdigit():
+ return float(retry_after)
+
+ return wait_random_exponential(min=API_RETRY_WAIT_MIN, max=API_RETRY_WAIT_MAX)(retry_state)
Review Comment:
It doesn't, no.
The migration here is a direct port-over of what retryhttp did. It had support for 429 even when API server doesn't support it as of today -- maybe some day? Not so sure if we want to keep support since its a no-op for client now.
`Retry-After` is often a common header sent for rate limit status codes: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/429
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3433929827
> @kaxil I have added this to 3.1.2 (we could also consider 3.2), but do you think there is a reason to add it to 3.1.1? I think not.
>
> @jscheffl I see similar patterns for edge executor too, you could adopt a similar pattern there too. LMK if you want me to take a stab at it.
Yes, I'd also apply this to edge but I am still hoping for the PR of Kaxil merged and then followed by https://github.com/austind/retryhttp/pull/28 as I'd like to change edge to use asyncio, then I'd need aiohttp support... and post the fix from Kaxil no dependency to requests anymore so can still keep retryhttp...
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2454156518
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,14 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retried based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ return exception.response.status_code >= 500
Review Comment:
It does the same.
Retryhttp retried these:
* HTTPStatusError with 5xx status codes (server errors)
* HTTPStatusError with 429 status (rate limiting)
* NetworkError, ConnectError, ReadError, WriteError (network issues)
* TimeoutException, ConnectTimeout, ReadTimeout (timeout issues)
Now, this happens:
```python
if isinstance(exception, httpx.HTTPStatusError):
return exception.response.status_code >= 500
```
handled retries for server errors (5xx)
and
```python
return isinstance(exception, httpx.RequestError)
```
handles network / timeout errors.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2454168668
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,14 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retried based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ return exception.response.status_code >= 500
Review Comment:
Ran a simple script for checking difference:
Testing _should_retry_api_request:
✅ HTTPStatusError: True (expected True)
✅ HTTPStatusError: True (expected True)
✅ HTTPStatusError: True (expected True)
✅ HTTPStatusError: False (expected False)
✅ HTTPStatusError: False (expected False)
✅ HTTPStatusError: False (expected False)
✅ HTTPStatusError: False (expected False)
✅ ConnectError: True (expected True)
✅ ReadTimeout: True (expected True)
✅ WriteError: True (expected True)
✅ ProxyError: True (expected True)
✅ ValueError: False (expected False)
✅ RuntimeError: False (expected False)
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2453164267
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,14 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retried based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ return exception.response.status_code >= 500
Review Comment:
The checks in retryhttp are a bit more explicit, have you considered replicating the same list instread of just >= 500?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2440106789
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,37 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retrie based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ status = exception.response.status_code
+ return status >= 500 or status == 429
+
+ # for all other httpx errors (network, timeout, connect, etc.), retry
+ if isinstance(exception, httpx.RequestError):
+ return True
+
+ return False
+
+
+def _get_retry_wait_time(retry_state) -> float:
+ """
+ Calculate wait time for retry, respecting Retry-After header on 429 responses.
+
+ For rate limit responses (429) with a Retry-After header, uses the value from the header.
+ Otherwise, fall bacsk to exponential backoff with jitter.
+ """
+ exception = retry_state.outcome.exception()
+
+ if isinstance(exception, httpx.HTTPStatusError):
+ if exception.response.status_code == 429:
+ retry_after = exception.response.headers.get("Retry-After")
+ if retry_after and retry_after.isdigit():
+ return float(retry_after)
+
+ return wait_random_exponential(min=API_RETRY_WAIT_MIN, max=API_RETRY_WAIT_MAX)(retry_state)
Review Comment:
YAGNI
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2440580120
##########
task-sdk/tests/task_sdk/api/test_client.py:
##########
@@ -225,6 +225,42 @@ def test_retry_handling_ok(self, mock_sleep):
assert len(responses) == 1
assert mock_sleep.call_count == 0
+ @mock.patch("time.sleep", return_value=None)
+ def test_retry_handling_network_error(self, mock_sleep):
+ """Test that network errors trigger retry and eventually recover."""
+ call_count = 0
+
+ def handle_request(request: httpx.Request) -> httpx.Response:
+ nonlocal call_count
+ call_count += 1
+ if call_count < 3:
+ raise httpx.NetworkError("Connection failed")
+ return httpx.Response(200, json={"detail": "Recovered from error"})
+
+ client = make_client(transport=httpx.MockTransport(handle_request))
+ response = client.get("http://error")
+ assert response.status_code == 200
+ assert call_count == 3
+ assert mock_sleep.call_count == 2
+
+ @mock.patch("time.sleep", return_value=None)
Review Comment:
Same as above -- better to use time machine
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #56762:
URL: https://github.com/apache/airflow/pull/56762#discussion_r2440123755
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -812,6 +817,37 @@ def noop_handler(request: httpx.Request) -> httpx.Response:
API_SSL_CERT_PATH = conf.get("api", "ssl_cert")
+def _should_retry_api_request(exception: BaseException) -> bool:
+ """Determine if an API request should be retrie based on the exception type."""
+ if isinstance(exception, httpx.HTTPStatusError):
+ status = exception.response.status_code
+ return status >= 500 or status == 429
+
+ # for all other httpx errors (network, timeout, connect, etc.), retry
+ if isinstance(exception, httpx.RequestError):
+ return True
+
+ return False
+
+
+def _get_retry_wait_time(retry_state) -> float:
+ """
+ Calculate wait time for retry, respecting Retry-After header on 429 responses.
+
+ For rate limit responses (429) with a Retry-After header, uses the value from the header.
+ Otherwise, fall bacsk to exponential backoff with jitter.
+ """
+ exception = retry_state.outcome.exception()
+
+ if isinstance(exception, httpx.HTTPStatusError):
+ if exception.response.status_code == 429:
+ retry_after = exception.response.headers.get("Retry-After")
+ if retry_after and retry_after.isdigit():
+ return float(retry_after)
+
+ return wait_random_exponential(min=API_RETRY_WAIT_MIN, max=API_RETRY_WAIT_MAX)(retry_state)
Review Comment:
Of course!
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Migrate retry handler in task SDK API client to use tenacity instead of retryhttp [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #56762:
URL: https://github.com/apache/airflow/pull/56762#issuecomment-3416850640
Yeah looking at it! Also have a gap in retryhttp as I would like to switch to aiohttp in edge and thought about how to continue. Might be I follow your path...
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org