Nataneljpwd commented on code in PR #63016:
URL: https://github.com/apache/airflow/pull/63016#discussion_r2897930731
##########
airflow-ctl/docs/cli-and-env-variables-ref.rst:
##########
@@ -60,3 +60,21 @@ Environment Variables
It disables some features such as keyring integration and save credentials
to file.
It is only meant to use if either you are developing airflowctl or running
API integration tests.
Please do not use this variable unless you know what you are doing.
+
+.. envvar:: AIRFLOW_CLI_API_RETRIES
+
+ The number of times to retry an API call if it fails. This is
+ only used if you are using the Airflow API and have not set up
+ authentication using a different method. The default value is 3.
+
+.. envvar:: AIRFLOW_CLI_API_RETRY_WAIT_MIN
+
+ The minimum amount of time to wait between API retries in seconds.
+ This is only used if you are using the Airflow API and have not set up
+ authentication using a different method. The default value is 1 second.
+
+.. envvar:: AIRFLOW_CLI_API_RETRY_WAIT_MAX
+
+ The maximum amount of time to wait between API retries in seconds.
+ This is only used if you are using the Airflow API and have not set up
+ authentication using a different method. The default value is 10 seconds.
Review Comment:
Why isn't it a static number? I think it would be simpler to just have a
static number if there is not crucial reason for it
##########
airflow-ctl/tests/airflow_ctl/api/test_client.py:
##########
@@ -314,3 +324,55 @@ def test_save_skips_patch_for_non_encrypted_backend(self,
mock_keyring):
assert not hasattr(mock_backend, "_get_new_password")
mock_keyring.set_password.assert_called_once_with("airflowctl",
"api_token_production", "token")
+
+ def test_retry_handling_unrecoverable_error(self):
+ with time_machine.travel("2023-01-01T00:00:00Z", tick=False):
+ responses: list[httpx.Response] = [
+ *[httpx.Response(500, text="Internal Server Error")] * 6,
+ httpx.Response(200, json={"detail": "Recovered from error -
but will fail before"}),
+ httpx.Response(400, json={"detail": "Should not get here"}),
+ ]
+ client = make_client_w_responses(responses)
+
+ with pytest.raises(httpx.HTTPStatusError) as err:
+ client.get("http://error";)
+ assert not isinstance(err.value, ServerResponseError)
+ assert len(responses) == 5
+
Review Comment:
Maybe I would also move the client utility closer to this test, as it is
used only in these tests, it will make it much easier to understand by reducing
the mental load
##########
airflow-ctl/tests/airflow_ctl/api/test_client.py:
##########
@@ -314,3 +324,55 @@ def test_save_skips_patch_for_non_encrypted_backend(self,
mock_keyring):
assert not hasattr(mock_backend, "_get_new_password")
mock_keyring.set_password.assert_called_once_with("airflowctl",
"api_token_production", "token")
+
+ def test_retry_handling_unrecoverable_error(self):
+ with time_machine.travel("2023-01-01T00:00:00Z", tick=False):
+ responses: list[httpx.Response] = [
+ *[httpx.Response(500, text="Internal Server Error")] * 6,
+ httpx.Response(200, json={"detail": "Recovered from error -
but will fail before"}),
+ httpx.Response(400, json={"detail": "Should not get here"}),
+ ]
+ client = make_client_w_responses(responses)
+
+ with pytest.raises(httpx.HTTPStatusError) as err:
+ client.get("http://error";)
+ assert not isinstance(err.value, ServerResponseError)
+ assert len(responses) == 5
+
Review Comment:
I think that to make these tests simpler to understand it is possible to add
comments to the asserts to describe what the numbers mean
--
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]
