Add missing AUTOINC/SERIAL for FAB tables #26885
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kaxil
reviewed
Oct 5, 2022
airflow/migrations/versions/0118_2_4_2_add_missing_autoinc_fab.py
Outdated
Show resolved
Hide resolved
kaxil
approved these changes
Oct 5, 2022
ashb
commented
Oct 5, 2022
airflow/migrations/versions/0118_2_4_2_add_missing_autoinc_fab.py
Outdated
Show resolved
Hide resolved
jedcunningham
approved these changes
Oct 5, 2022
airflow/migrations/versions/0118_2_4_2_add_missing_autoinc_fab.py
Outdated
Show resolved
Hide resolved
blag
approved these changes
Oct 5, 2022
airflow/migrations/versions/0118_2_4_2_add_missing_autoinc_fab.py
Outdated
Show resolved
Hide resolved
uranusjr
force-pushed
the
fab-tables-missing-autoinc-pre-1-10-12
branch
from
e34f48c
to
43af374
Compare
uranusjr
approved these changes
Oct 6, 2022
kaxil
force-pushed
the
fab-tables-missing-autoinc-pre-1-10-12
branch
from
43af374
to
748a7c9
Compare
potiuk
approved these changes
Oct 6, 2022
2 tasks
2 tasks
2 tasks
blag
approved these changes
Oct 6, 2022
In 1.10.13 we introduced a migration that creates the tables with the server_default but that migration only did anything if the tables didn't already exist. But the tables created by the FAB model have a default (but not a server_default). Oh, and the final bit of the puzzle, in 2.4 we finally "took control" of the FAB security models in to airflow and those do not have the default set.
ashb
force-pushed
the
fab-tables-missing-autoinc-pre-1-10-12
branch
from
748a7c9
to
60d01a9
Compare
|
The change to make alembic re-use the existing connection didn't fix the problem because we have some migrations that make use of 🤔 |
Without this `create_session()` will open a new connection, and that causes mysql to hang waiting to get a "metadata lock on table". Using the "stock" pool with size=1 and max_overflow=0 doesn't work, that instead times out if you try to get a new connection from the pool. SingletonThreadPool instead returns the existing active connection which is what we want.
ashb
force-pushed
the
fab-tables-missing-autoinc-pre-1-10-12
branch
from
4f21833
to
9981a9f
Compare
ephraimbuddy
pushed a commit
that referenced
this pull request
Oct 18, 2022
* Add missing AUTOINC/SERIAL for FAB tables In 1.10.13 we introduced a migration that creates the tables with the server_default but that migration only did anything if the tables didn't already exist. But the tables created by the FAB model have a default (but not a server_default). Oh, and the final bit of the puzzle, in 2.4 we finally "took control" of the FAB security models in to airflow and those do not have the default set. * Update airflow/migrations/versions/0118_2_4_2_add_missing_autoinc_fab.py * Fix static checks * Run migrations with with a pool of a connection. Without this `create_session()` will open a new connection, and that causes mysql to hang waiting to get a "metadata lock on table". Using the "stock" pool with size=1 and max_overflow=0 doesn't work, that instead times out if you try to get a new connection from the pool. SingletonThreadPool instead returns the existing active connection which is what we want. Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> (cherry picked from commit 7efdeed)
37 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In 1.10.13 we introduced a migration that creates the tables with the server_default but that migration only did anything if the tables didn't already exist. But the tables created by the FAB model have a default (but not a server_default).
Oh, and the final bit of the puzzle, in 2.4 we finally "took control" of the FAB security models in to airflow and those do not have the default set.
Fixes #26497