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: catalog upgrade/downgrade #29780

Merged
merged 1 commit into from
Jul 30, 2024
Merged

fix: catalog upgrade/downgrade #29780

merged 1 commit into from
Jul 30, 2024

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Jul 30, 2024

SUMMARY

Many little improvements to the catalog migration helpers, especially on the downgrade.

  1. Works when the analytical DB is unreachable (fixed for schemas in fix: make catalog migration lenient #29549, this PR fixes catalogs)
  2. Delete catalog permissions on downgrade.
  3. Delete non-default catalog schema permissions on downgrade.
  4. Delete models on non-default catalog models.

The exception to (1) is that on upgrade we need to run database.get_default_catalog() in order to update models; doing this later would be messy and could lead to security issues. The get_default_catalog() method usually doesn't need to connect to the analytical database, except for Databricks (where it's always the need) and BigQuery (when the project is not specified in the SQLAlchemy URI).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

Updated and improved tests.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@betodealmeida betodealmeida requested a review from a team as a code owner July 30, 2024 16:47
@github-actions github-actions bot added the risk:db-migration PRs that require a DB migration label Jul 30, 2024
@dosubot dosubot bot added change:backend Requires changing the backend data:databases Related to database configurations and connections labels Jul 30, 2024
@michael-s-molina michael-s-molina added the review:checkpoint Last PR reviewed during the daily review standup label Jul 30, 2024
@michael-s-molina michael-s-molina linked an issue Jul 30, 2024 that may be closed by this pull request
3 tasks
# more than 50 different database drivers. Usually the try/except block will catch the
# generic `Exception` class, which requires a pylint disablee comment. To make it clear
# that we know this is a necessary evil we create an alias, and catch it instead.
GenericDBException = Exception
Copy link
Member

Choose a reason for hiding this comment

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

why not ->

class GenericDBException(Exception):
    pass

Copy link
Member

Choose a reason for hiding this comment

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

nevermind, I understand now reading the code

Copy link
Member Author

@betodealmeida betodealmeida Jul 30, 2024

Choose a reason for hiding this comment

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

In that case the exceptions from all the DB API drivers would not be derived from GenericDBException, so we wouldn't be able to catch them.

The only exception that is a parent to all the exceptions from the multitude of DB API drivers we support is Exception itself (I hope!), which is why we have to write:

try:
    # do something with some dynamically loaded DB API driver
except Exception:  # pylint: disable=broad-except
    pass

It's inevitable to have a broad except, because the drivers are loaded dynamically. My alias is just a way of saying "I know this is bad, but what can I do?", but also "I'm expecting an exception here that could be anything".

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

I did a pass on this and overall LGTM

@betodealmeida betodealmeida merged commit 525e837 into master Jul 30, 2024
37 checks passed
@sadpandajoe
Copy link
Member

@supersetbot label 4.1

@github-actions github-actions bot added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Jul 30, 2024
@rusackas rusackas deleted the catalog-migration branch July 30, 2024 22:06
sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Jul 30, 2024
@michael-s-molina michael-s-molina removed the review:checkpoint Last PR reviewed during the daily review standup label Jul 31, 2024
sadpandajoe pushed a commit to preset-io/superset that referenced this pull request Aug 1, 2024
sadpandajoe pushed a commit that referenced this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change:backend Requires changing the backend data:databases Related to database configurations and connections preset-io risk:db-migration PRs that require a DB migration size/XL v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4.1.0rc1 migration fails if there is unreachable database.
4 participants