You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "bugraoz93 (via GitHub)" <gi...@apache.org> on 2026/03/06 20:46:04 UTC
[PR] Add retry mechanism to airflowctl and remove flaky integration mark [airflow]
bugraoz93 opened a new pull request, #63016:
URL: https://github.com/apache/airflow/pull/63016
closes: #60421
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: http://chris.beams.io/posts/git-commit/
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 (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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] Add retry mechanism to airflowctl and remove flaky integration mark [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #63016:
URL: https://github.com/apache/airflow/pull/63016#issuecomment-4016110475
CI not related
--
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] Add retry mechanism to airflowctl and remove flaky integration mark [airflow]
Posted by "Nataneljpwd (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add retry mechanism to airflowctl and remove flaky integration mark [airflow]
Posted by "Nataneljpwd (via GitHub)" <gi...@apache.org>.
Nataneljpwd commented on code in PR #63016:
URL: https://github.com/apache/airflow/pull/63016#discussion_r2900422703
##########
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:
Yes, it is clear but it takes a second to understand, it is a nice to have rather than a must
--
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] Add retry mechanism to airflowctl and remove flaky integration mark [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #63016:
URL: https://github.com/apache/airflow/pull/63016#discussion_r2906814804
##########
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:
We will cut the version today or tomorrow. To include into next release, I will merge it. I am also thinking that test names are self-explanatory. So as not to add more LoC into the repository, will keep it as is. Thanks for your reviews :)
--
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] Add retry mechanism to airflowctl and remove flaky integration mark [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #63016:
URL: https://github.com/apache/airflow/pull/63016#discussion_r2899644661
##########
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 it's quite clear what those tests 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add retry mechanism to airflowctl and remove flaky integration mark [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #63016:
URL: https://github.com/apache/airflow/pull/63016#discussion_r2899418416
##########
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:
This can really depend on your environment. This also customisable in TaskSDK. You can have downgraded performance while then airflowctl won't be useful maybe with 1 second. That's why givin flexibility to user over these never hurts
--
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] Add retry mechanism to airflowctl and remove flaky integration mark [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #63016:
URL: https://github.com/apache/airflow/pull/63016#discussion_r2899421324
##########
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:
Utility is also defined in the test. Only this test file updated and the method added in client which should be in the client test.
Can you elaborate?
--
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] Add retry mechanism to airflowctl and remove flaky integration mark [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 merged PR #63016:
URL: https://github.com/apache/airflow/pull/63016
--
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