-
Notifications
You must be signed in to change notification settings - Fork 14.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AIRFLOW-5343] Remove legacy way of pessimistic disconnect handling #6034
Conversation
|
||
# How many seconds to retry re-establishing a DB connection after | ||
# disconnects. Setting this to 0 disables retries. | ||
sql_alchemy_reconnect_timeout = 300 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens now with disconnects/errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If connection from the pool is not healthy, it's removed from the pool and new one is created (with three attempts). Other connections are invalidated from the pool as well.
sqlalchemy/sqlalchemy@f881dae#diff-31816cdb15e64b0af1b862f51abe1226R920 - the test visualizes this.
This commit from SQLAlchemy is quite a good explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pool_pre_ping argument should take care of it and the code for re connect is within sql alchemy library. We don't have to handle explicitly with new sql alchemy version
Based on discussions in #5949 it was figured out that there is already pessimistic disconnect timeout handling. So instead of hand-written one only SQLAlchemy embedded way should be used. 'sqlalchemy~=1.3' is in `setup.py` requirements and `pool_pre_ping` appeared in SQLAlchemy 1.2.
Codecov Report
@@ Coverage Diff @@
## master #6034 +/- ##
==========================================
- Coverage 80.03% 78.72% -1.32%
==========================================
Files 594 594
Lines 34748 35248 +500
==========================================
- Hits 27812 27749 -63
- Misses 6936 7499 +563
Continue to review full report at Codecov.
|
Codecov report is confusing in terms of coverage. There are mainly code deletions here. |
…pache#6034) Based on discussions in apache#5949 it was figured out that there is already pessimistic disconnect timeout handling. So instead of hand-written one only SQLAlchemy embedded way should be used. 'sqlalchemy~=1.3' is in `setup.py` requirements and `pool_pre_ping` appeared in SQLAlchemy 1.2. (cherry picked from commit bc82607)
…pache#6034) Based on discussions in apache#5949 it was figured out that there is already pessimistic disconnect timeout handling. So instead of hand-written one only SQLAlchemy embedded way should be used. 'sqlalchemy~=1.3' is in `setup.py` requirements and `pool_pre_ping` appeared in SQLAlchemy 1.2. (cherry picked from commit bc82607)
Make sure you have checked all steps below.
Jira
Description
Based on discussions in #5949 it was figured out that there is already pessimistic disconnect timeout handling. So instead of hand-written one only SQLAlchemy embedded way should be used.
'sqlalchemy~=1.3' is in
setup.py
requirements andpool_pre_ping
appeared in SQLAlchemy 1.2.Tests
It's quite an edge case for integrated environment. I'm not quite sure how to test it.
Commits
Documentation