Add --retry and --retry-delay to "airflow db check"#31836
Conversation
c0dfa07 to
ea74388
Compare
There was a problem hiding this comment.
I think we should give the database more time to breathe. :)
Would it make sense to increase the default value to 30 or something more?
There was a problem hiding this comment.
The db check retry here is really targetted to see if the database is "up" already - nothing more.
I think 1s is quite good as a default. We do not do a LOT - we just try to connect to the database which takes milliseconds usually. And if it does not work, then it usually means that the database is not yet ready and we will simply fail on failed "connection" establishing - nothing that makes it "heavy".
The longer the delay is, the longer we will delay startup if this command is used as a prerequisite to start airflow component (and this is the intention).
There was a problem hiding this comment.
Sure, that makes sense. :)
There was a problem hiding this comment.
You can also consider using - https://tenacity.readthedocs.io/en/latest/ for retries it gives you many options
There was a problem hiding this comment.
yeah. using tenacity (especially that we use it, is a good idea here).
There was a problem hiding this comment.
Thank you for the suggestion. Will work on incorporating tenacity right away.
24a2503 to
a6ae9be
Compare
There was a problem hiding this comment.
@potiuk @utkarsharma2
Since tenacity.retry relies on unhandled exceptions for indicating whether a function call (or a code block in this case) is successful or not, I've reverted the implementation of airflow.utils.db.check back to its original state (letting the SQLAlchemy session raise unhandled OperationalError or else). Consequently, there is no longer any need to unit test this util function.
I chose to set reraise=True so that the original error from airflow.utils.db.check can be surfaced. Please advise whether we should raise RetryError or the original error. Thank you.
Last but not least, the unit test for db_command.check is updated accordingly. I am not 100% sure if testing the retry logic implemented in tenacity.retry is all that helpful, but the test itself still mocks time.sleep and utils.db.check and test that they are called for the correct number of times.
There was a problem hiding this comment.
Yup, there is no need to test for tenacity code.
Also, reraising the original exception is vital as it gives users a hint of what needs to be done next, whereas RetryError would be meaningless to them.
There was a problem hiding this comment.
Yes. That's the nice thing about using small and proven utils. No need to test them :)
a6ae9be to
1055ec2
Compare
511d55b to
74834f6
Compare
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
LGTM. Nice, smalll improvement . Good job @xuganyu96
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
4 participants
This PR closes #31818. It is still a work in progress with many to-do's pending:
This PR made significant refactor to
airflow.utils.db.checkfunction, which I felt safe to do because I found this function to only be used by thedb checkcommand, and which I felt it not an API used externally. Please let me know if this approach is not satisfactory.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.