Skip to content

Conversation

@michael-s-molina
Copy link
Member

SUMMARY

Depending on the amount of data an organization has, the migration scripts can take a long time to run. This PR adds the duration of the complete migration process to help administrators to estimate downtime for upgrades/downgrades. Duration is only logged in case all migrations successfully complete.

TESTING INSTRUCTIONS

CI should be sufficient.

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

@michael-s-molina michael-s-molina requested a review from a team as a code owner August 8, 2024 14:53
@github-actions github-actions bot added the risk:db-migration PRs that require a DB migration label Aug 8, 2024
@dosubot dosubot bot added the change:backend Requires changing the backend label Aug 8, 2024
@michael-s-molina michael-s-molina added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Aug 8, 2024
@codecov
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

❌ Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.69%. Comparing base (76d897e) to head (8fe12f2).
⚠️ Report is 2419 commits behind head on master.

Files with missing lines Patch % Lines
superset/migrations/env.py 0.00% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #29893       +/-   ##
===========================================
+ Coverage   60.48%   83.69%   +23.20%     
===========================================
  Files        1931      527     -1404     
  Lines       76236    38041    -38195     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    31839    -14275     
+ Misses      28017     6202    -21815     
+ Partials     2105        0     -2105     
Flag Coverage Δ
hive ∅ <0.00%> (∅)
javascript ?
mysql ∅ <0.00%> (?)
postgres ∅ <0.00%> (?)
presto ∅ <0.00%> (∅)
python ∅ <0.00%> (∅)
sqlite ∅ <0.00%> (?)
unit ∅ <0.00%> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@michael-s-molina michael-s-molina removed the risk:db-migration PRs that require a DB migration label Aug 8, 2024
Copy link
Contributor

@giftig giftig left a comment

Choose a reason for hiding this comment

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

A couple of minor nitpicks

@github-actions github-actions bot added the risk:db-migration PRs that require a DB migration label Aug 9, 2024
@michael-s-molina michael-s-molina requested a review from giftig August 9, 2024 13:22
@michael-s-molina michael-s-molina removed the risk:db-migration PRs that require a DB migration label Aug 9, 2024
@github-actions github-actions bot added the risk:db-migration PRs that require a DB migration label Aug 9, 2024
@michael-s-molina michael-s-molina force-pushed the add-migrations-elapsed-time branch from e6b2122 to 8fe12f2 Compare August 9, 2024 13:25
@michael-s-molina michael-s-molina removed the risk:db-migration PRs that require a DB migration label Aug 9, 2024
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.

[optional] a context decorator could be nice and simple for this:

with print_duration(label=f"Migration: {migration_id}"):
   do_stuff()

Wondering if just using event_logger.log_context(action='migration') would just work (in CI, the logging should be utils.log.StdOutEventLogger, though it doesn't seem to be the default)

@michael-s-molina
Copy link
Member Author

[optional] a context decorator could be nice and simple for this:

That would be nice. We can think about this when creating the helper functions for the migrations as part of [SIP-142] Improving Database Migration Management

@michael-s-molina michael-s-molina merged commit 57a4199 into apache:master Aug 9, 2024
sadpandajoe pushed a commit that referenced this pull request Aug 13, 2024
@github-actions github-actions bot added 🍒 4.1.0 Cherry-picked to 4.1.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Nov 14, 2024
@mistercrunch mistercrunch added the 🍒 4.1.1 Cherry-picked to 4.1.1 label Nov 27, 2024
@github-actions github-actions bot added the 🍒 4.1.2 Cherry-picked to 4.1.2 label Apr 1, 2025
@mistercrunch mistercrunch added 🍒 4.1.3 Cherry-picked to 4.1.3 🚢 5.0.0 First shipped in 5.0.0 labels Jul 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels change:backend Requires changing the backend size/S v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch 🍒 4.1.0 Cherry-picked to 4.1.0 🍒 4.1.1 Cherry-picked to 4.1.1 🍒 4.1.2 Cherry-picked to 4.1.2 🍒 4.1.3 Cherry-picked to 4.1.3 🍒 4.1.4 🚢 5.0.0 First shipped in 5.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants