You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "notatallshaw-gts (via GitHub)" <gi...@apache.org> on 2023/05/04 21:16:02 UTC
[GitHub] [airflow] notatallshaw-gts opened a new pull request, #31073: Add default retry to hvac requests
notatallshaw-gts opened a new pull request, #31073:
URL: https://github.com/apache/airflow/pull/31073
This implemented a retry mechanism to the hvac client as the libraries [documentation suggests](https://hvac.readthedocs.io/en/stable/advanced_usage.html#retrying-failed-requests).
I am providing this due to a real-world case of an HTTP Connection Error causing a task to fail to start, after investigating I found the hvac documentation suggested adding this retry process.
Due to the docker version I have access to and the minimum requirements of Airflow dev tooling I hit limitations in the MyPy pre-commits and running the test suite, but I believe this PR should pass all tests.
Let me know if you need any additional work and/or changes to the PR to be able to review and/or land.
Finally, someone else submitted a similar PR but it is failing due import errors and they have not updated the PR since it was created three weeks ago: https://github.com/apache/airflow/pull/30628
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.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 [newsfragments](https://github.com/apache/airflow/tree/main/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
[GitHub] [airflow] potiuk commented on pull request #31073: Add default retry to hvac client requests
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #31073:
URL: https://github.com/apache/airflow/pull/31073#issuecomment-1599471744
> @hussein-awala @potiuk Anything further needed to do for this to be merged?
Atttention grabbing of those who could merge it (which BTW. you just did).
--
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
[GitHub] [airflow] notatallshaw-gts commented on pull request #31073: Add default retry to hvac client requests
Posted by "notatallshaw-gts (via GitHub)" <gi...@apache.org>.
notatallshaw-gts commented on PR #31073:
URL: https://github.com/apache/airflow/pull/31073#issuecomment-1535463020
Ah, the additional asset call tests are failing. I guess I can fix them by adding an expected `session=client.kwargs['session']` but maybe there's a more elegant solution, I will think about.
--
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
[GitHub] [airflow] notatallshaw-gts commented on pull request #31073: Add default retry to hvac client requests
Posted by "notatallshaw-gts (via GitHub)" <gi...@apache.org>.
notatallshaw-gts commented on PR #31073:
URL: https://github.com/apache/airflow/pull/31073#issuecomment-1583245048
> conflicts to resolve
Thanks for the heads up, I will fix.
Apologies for not keeping to my earlier estimate of providing tests. I will look to get this PR ready ASAP given my commitments.
--
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
[GitHub] [airflow] notatallshaw-gts commented on pull request #31073: Add default retry to hvac client requests
Posted by "notatallshaw-gts (via GitHub)" <gi...@apache.org>.
notatallshaw-gts commented on PR #31073:
URL: https://github.com/apache/airflow/pull/31073#issuecomment-1546786471
I haven't had chance to add new tests yet, I will try and add them tomorrow or Monday
--
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
[GitHub] [airflow] notatallshaw-gts commented on pull request #31073: Add default retry to hvac client requests
Posted by "notatallshaw-gts (via GitHub)" <gi...@apache.org>.
notatallshaw-gts commented on PR #31073:
URL: https://github.com/apache/airflow/pull/31073#issuecomment-1588152710
> Needs conflict resolving
Resolved conflict and added new test to check default retry is added to hvac client. Thanks for your time, let me know if anything else needs to be done to merge.
--
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
[GitHub] [airflow] potiuk commented on pull request #31073: Add default retry to hvac client requests
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #31073:
URL: https://github.com/apache/airflow/pull/31073#issuecomment-1583365045
Needs conflict resolving
--
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
[GitHub] [airflow] notatallshaw-gts commented on pull request #31073: Add default retry to hvac client requests
Posted by "notatallshaw-gts (via GitHub)" <gi...@apache.org>.
notatallshaw-gts commented on PR #31073:
URL: https://github.com/apache/airflow/pull/31073#issuecomment-1598966740
@hussein-awala @potiuk Anything further needed to do for this to be merged?
--
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
[GitHub] [airflow] notatallshaw-gts commented on pull request #31073: Add default retry to hvac client requests
Posted by "notatallshaw-gts (via GitHub)" <gi...@apache.org>.
notatallshaw-gts commented on PR #31073:
URL: https://github.com/apache/airflow/pull/31073#issuecomment-1536484661
> IMO this needs to be tested, even if it looks trivial. We should avoid the possibility of someone accidentally breaking it. You can add two tests, one with the session argument and one without.
Agreed, I'll create some new tests (fyi it may take me a few days to update this when I have the time to work on it)
> And for the failed tests, since there is no argument to activate/deactivate the retry (we can do this by providing a None session), you should fix all of them.
Sure I can do this
> I think we can replace the `mock_hvac.Client.assert_called_with` by getting the call args and compare the urls, otherwise you need to get the session from the kwargs and compare it to the passed session, which is useless.
I'm not sure I follow, if I fix the tests by passing `session=None` as stated above what do you want to do with a url?
--
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
[GitHub] [airflow] potiuk merged pull request #31073: Add default retry to hvac client requests
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #31073:
URL: https://github.com/apache/airflow/pull/31073
--
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
[GitHub] [airflow] potiuk commented on pull request #31073: Add default retry to hvac client requests
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #31073:
URL: https://github.com/apache/airflow/pull/31073#issuecomment-1583204731
conflicts to resolve
--
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