Skip to content
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

Fix migration metadata lock on ab_view_menu #7417

Closed
wants to merge 3 commits into from
Closed

Fix migration metadata lock on ab_view_menu #7417

wants to merge 3 commits into from

Conversation

jdbertron
Copy link

This fix is to make sure the scoped session created by the superset
init code (that creates a bunch of roles and permissions) will
be released and potentially committed when the superset/init.py
goes out of scope.

modified:   superset/__init__.py

CATEGORY

Choose one

  • [ X] Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Without running in a context, the code leaves the session open with a lock on ab_view_menu
metadata, which prevents migration cefabc8f7d38_increase_size_of_name_column_in_ab_view
from running an ALTER on that table.
Another contributor suggested explicitly closing the session or trying to use the same session inside the migration code. This requires more engineering and the sm.session.close() would have to be explicit. Any new interaction with the database would cause the problem to happen again.
See here: #1260 (comment)
I found using an app context to work better to encapsulate the superset init code, in case people add more logic there, the session close will happen automatically as the context goes out of scope.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Run migrations using mysql to version 3e1b21cd94a4
Run migrations to cefabc8f7d38
Without the fix, it should hang on the ALTER ab_view_menu, and with the fix it will complete.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

This fix is to make sure the scoped session created by the AppBuilder
init code (that creates a bunch of roles and permissions) will
be released and committed when the AppBuidler init code
goes out of scope. Without running in a context, the
code leaves the session open with a lock on ab_view_menu
metadata, which prevents migration cefabc8f7d38_increase_size_of_name_column_in_ab_view
from running an ALTER on that table.

	modified:   superset/__init__.py
@mistercrunch
Copy link
Member

Since recently (FAB 2.0), FAB supports using the Flask application factory pattern http://flask.pocoo.org/docs/1.0/patterns/appfactories/

We should move to using this for obvious reasons.

In the meantime, @dpgaspar (FAB creator/maintainer) what do you think about this?

@mistercrunch
Copy link
Member

Ooops there's a conflict here.

@craig-rueda
Copy link
Member

👍 we're seeing this issue too :)

superset/__init__.py Outdated Show resolved Hide resolved
Copy link
Author

@jdbertron jdbertron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Craig was right. I made the change.

superset/__init__.py Outdated Show resolved Hide resolved
@amodig
Copy link

amodig commented May 13, 2019

Hi, my superset db upgrade hangs up on the following migration:
INFO [alembic.runtime.migration] Running upgrade 6c7537a6004a -> cefabc8f7d38, Increase size of name column in ab_view_menu
I guess it's related to this bug? I tried to fix the issue by upgrading Superset from jdbertron:master but it didn't help. Would you be able to help me out? I'm using Google Cloud MySQL.

@stale
Copy link

stale bot commented Jul 12, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Jul 12, 2019
@stale stale bot removed the inactive Inactive for >= 30 days label Jul 12, 2019
@mistercrunch
Copy link
Member

Do we still need this?

@codecov-io
Copy link

Codecov Report

Merging #7417 into master will decrease coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7417      +/-   ##
==========================================
- Coverage    65.8%   65.65%   -0.15%     
==========================================
  Files         461      461              
  Lines       22090    22090              
  Branches     2422     2422              
==========================================
- Hits        14536    14504      -32     
- Misses       7433     7465      +32     
  Partials      121      121
Impacted Files Coverage Δ
superset/db_engine_specs/mysql.py 34.88% <0%> (-58.14%) ⬇️
superset/models/core.py 81.8% <0%> (-0.66%) ⬇️
superset/views/core.py 71.84% <0%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ebc5a6...bd74a3c. Read the comment docs.

@mistercrunch
Copy link
Member

Actually just realized that a very similar PR was merged, no need for this anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants