fix: set default mysql isolation level to 'READ COMMITTED' #30174
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
#28628 had the virtuous intent to align mysql and postgres to a consistent isolation_level (READ COMMITTED), which seems fitted for Superset. Though instead of doing this, and because sqlite doesn't support that specific one, it set the default to SERIALIZABLE which seems to exist by many engines. Here I'm realizing we need dynamic defaults for isolation_level based on which engine is in used, which can't be done in config.py as we don't know yet what engine will be set by the admin at that time. So I thought the superset/initialization package might be the right place for this. Open to other solutions, but I think this one works. * using DB_CONNECTION_MUTATOR, but that one applies only to analytics workloads, not the metadata db
This was referenced Sep 6, 2024
villebro
reviewed
Sep 6, 2024
superset/initialization/__init__.py
Outdated
Comment on lines
498
to
500
| if not isolation_level and self.config["SQLALCHEMY_DATABASE_URI"].startswith( | ||
| "mysql" | ||
| ): |
There was a problem hiding this comment.
I kinda liked John's cleaner way of extracting the backend name:
backend = make_url(SQLALCHEMY_DATABASE_URI).get_backend_name()Also, weren't we supposed to do READ COMMITTED by default for Postgres, too?
There was a problem hiding this comment.
ok, I addressed comments, lover how pre-commit caught something around make_url not being safe to use so I found I can use make_url_safe
villebro
reviewed
Sep 6, 2024
Co-authored-by: Ville Brofeldt <[email protected]>
dosubot
bot
added
data:connect:mysql
3 tasks
|
Wondering since the original pr is a breaking change if we should pull it out of 4.1 and wait until 5.0 to incorporate it. |
|
@villebro do you think that's mergeable? |
villebro
approved these changes
Sep 9, 2024
| @@ -1 +1,2 @@ | |||
| docker/**/*.sh text eol=lf | |||
| *.svg binary | |||
There was a problem hiding this comment.
Yup, Ill search for say a model name and the erd.svg muddies my git grep
sadpandajoe
added
the
v4.1
sadpandajoe
pushed a commit
that referenced
this pull request
Sep 10, 2024
Co-authored-by: Ville Brofeldt <[email protected]> (cherry picked from commit 6baeb65)
3 tasks
github-actions
bot
added
🍒 4.1.0
🏷️ 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.
#28628 had the virtuous intent to align mysql and postgres to a consistent isolation_level (READ COMMITTED), which seems fitted for Superset. Though instead of doing this, and because sqlite doesn't support that specific one, it set the default to SERIALIZABLE which seems to exist by many engines.
Now SERIALIZABLE can create issues like this one: #30064 and upping the isolation level can have serious load implications for large deployments (or databases that are fitted "just-right" for their loads, like someone may use a tiny RDS for Superset)
Here I'm realizing we need dynamic defaults for isolation_level based on which engine is in used, which can't be done in config.py as we don't know yet what engine will be set by the admin at that time. So I thought the superset/initialization package might be the right place for this.
Open to other solutions, but I think this one works.
alternative considered