Add HTTP retry handling into task SDK api.client #45121
Conversation
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
task_sdk/tests/api/test_client.py:101
- There is an extra single quote in the mounts dictionary key. It should be mounts={'http://': httpx.MockTransport(mock_handle_request)}.
mounts={'http://': httpx.MockTransport(mock_handle_request)},
jscheffl
force-pushed
the
feature/44355-add-retry-handling-into-task-sdk-api-client
branch
from
bc58ae6 to
5399ace
Compare
jscheffl
force-pushed
the
feature/44355-add-retry-handling-into-task-sdk-api-client
branch
from
5399ace to
23614f0
Compare
| API_RETRIES = int(os.getenv("AIRFLOW__WORKERS__API_RETRIES", 10)) | ||
| API_RETRY_WAIT_MIN = int(os.getenv("AIRFLOW__WORKERS__API_RETRY_WAIT_MIN", 1)) | ||
| API_RETRY_WAIT_MAX = int(os.getenv("AIRFLOW__WORKERS__API_RETRY_WAIT_MAX", 90)) |
There was a problem hiding this comment.
Can we also briefly document this? I think these are new env variables right?
There was a problem hiding this comment.
Do you have a proposal where to document? At the moment there is no config facility for Task SDK and also no section. Yes, the ENVs are new, ususally they are derived from airflow/config_templates/config.yml but for task SDK we explicitly don't want to force all backend config to be present in task SDK. Therefore the config mechanism was not used.
I just introduced these ENVs to have an option to configure and prevent hard-coding the retry values. More details are to be elaborated in #44352
| response = client.get("http://error") | ||
| assert response.status_code == 200 | ||
| assert len(responses) == 1 | ||
| assert mock_sleep.call_count == 0 | ||
|
|
||
|
|
||
| class TestTaskInstanceOperations: |
There was a problem hiding this comment.
Should we replace the client initialisation in all the other test classes to use make_client_w_responses instead?
There was a problem hiding this comment.
I briefly thought about it but if you take a look how the other tests are mocking the backend ... there are conditions and asserts in request handling implemented. So the other mock has a different test level. So I decided to move both close together but they have a different implementation demand.
jscheffl
force-pushed
the
feature/44355-add-retry-handling-into-task-sdk-api-client
branch
from
23614f0 to
aaabd09
Compare
closes: #44355
Add HTTP retry handling to Task SDK.