fix(db): use paginated_update for viz migration #20761
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
ktmud
force-pushed
the
use-paginated-update
branch
from
c63933b
to
f70a6fe
Compare
ktmud
changed the title
ktmud
force-pushed
the
use-paginated-update
branch
2 times, most recently
from
0f8233c
to
7484f25
Compare
ktmud
force-pushed
the
use-paginated-update
branch
from
7484f25
to
b67a49f
Compare
ktmud
commented
Jul 19, 2022
| @@ -45,11 +63,11 @@ def _migrate(self) -> None: | |||
|
|
|||
| rv_data = {} | |||
| for key, value in self.data.items(): | |||
| if key in self.mapping_keys and self.mapping_keys[key] in rv_data: | |||
| if key in self.rename_keys and self.rename_keys[key] in rv_data: | |||
| raise ValueError("Duplicate key in target viz") | |||
There was a problem hiding this comment.
I'm not sure if we should raise an error here. Maybe just silently override?
| source_viz_type: str | ||
| target_viz_type: str | ||
|
|
||
| def __init__(self, form_data: str) -> None: | ||
| self.data = json.loads(form_data) | ||
| self.data = try_load_json(form_data) |
There was a problem hiding this comment.
Data may be corrupted, let's always try/catch just to be safe.
| class MigrateVizEnum(str, Enum): | ||
| # the Enum member name is viz_type in database | ||
| treemap = "treemap" | ||
| area = "area" |
There was a problem hiding this comment.
Not need for such enum since we will import different stuff for different migrations anyway.
Codecov Report
@@ Coverage Diff @@
## master #20761 +/- ##
===========================================
- Coverage 66.35% 54.62% -11.74%
===========================================
Files 1754 1756 +2
Lines 66689 66721 +32
Branches 7049 7049
===========================================
- Hits 44253 36446 -7807
- Misses 20639 28478 +7839
Partials 1797 1797
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
zhaoyongjie
approved these changes
Jul 19, 2022
michael-s-molina
approved these changes
Jul 19, 2022
9 tasks
mistercrunch
added
🏷️ bot
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.
SUMMARY
DB migration introduced by #20359 did not run through in Airbnb environment and throws this error for our MySQL database:
This is because updating objects within
iter_perdoes not work with MySQL cursors. I guess the MySQL client just doesn't like writing while reading. (Maybe other databases will have similar issues if anyone with more than 1,000 area charts can test?)We've seen this error before, which was why
paginated_updatewas introduced. Instead of streaming results with cursors,paginated_updatewhich runs manual pagination withOFFSETandLIMIT, which makes sure read and write operations happen independently.While optimizing this, I also did some other refactoring for the migration_viz class. Most notably, I relocated the files (and the corresponding tests) from superset root to
superset.migrationsas migration code should be as self-contained and as stable as possible. Anything under the root directory is considered app code, and app code can be updated much more frequently.While testing, I also noticed that some charts will fail at reloading
query_contextas the combined JSON payload is too large for a default Text column in MySQL (which has max size of 64kb). Theupgradewas not able to save the full serialized JSON string---thendowngradewould fail. We need to migrate bothSlice.query_contextandSlice.paramstoMediumText, which I will address in another PR.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
CI and tested locally with Airbnb db instances.
The area chart migration was finished in ~20 seconds for about
ADDITIONAL INFORMATION