chore: Set isolation level to READ COMMITTED for testing et al. #28628

john-bodley · 2024-05-21T16:59:13Z

SUMMARY

The default isolation level for PostgreSQL is READ COMMITTED whereas for MySQL it is REPEATABLE READ. Ideally we would strive to have these the same (for consistency reasons) and the former is likely preferred given the async nature of Superset (especially SQL Lab) where the backend is polling to determine whether a query has been cancelled from another Flask-SQLAlchemy session.

This PR:

Adds a comment in config.py to try to persuade institutions to set the isolation level. It's actually hard/difficult to impose this given that the default config is configured for SQLite which doesn't support READ COMMITTED.
Updates the test configs to ensure that—for MySQL and PostgreSQL—the isolation level is READ COMMITTED.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

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

codecov · 2024-06-12T16:19:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.72%. Comparing base (76d897e) to head (6557097).
Report is 1094 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #28628       +/-   ##
===========================================
+ Coverage   60.48%   83.72%   +23.23%     
===========================================
  Files        1931      518     -1413     
  Lines       76236    37523    -38713     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    31416    -14698     
+ Misses      28017     6107    -21910     
+ Partials     2105        0     -2105

Flag	Coverage Δ
hive	`48.94% <100.00%> (-0.22%)`	⬇️
javascript	`?`
mysql	`77.22% <100.00%> (?)`
postgres	`77.33% <100.00%> (?)`
presto	`53.57% <100.00%> (-0.24%)`	⬇️
python	`83.72% <100.00%> (+20.23%)`	⬆️
sqlite	`76.79% <100.00%> (?)`
unit	`59.21% <100.00%> (+1.58%)`	⬆️

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.

john-bodley · 2024-06-12T22:42:26Z

tests/integration_tests/superset_test_config.py

@@ -52,12 +54,15 @@
 if "UPLOAD_FOLDER" in os.environ:  # noqa: F405
    UPLOAD_FOLDER = os.environ["UPLOAD_FOLDER"]  # noqa: F405

-if "sqlite" in SQLALCHEMY_DATABASE_URI:
+if make_url(SQLALCHEMY_DATABASE_URI).get_backend_name() == "sqlite":


Safer than doing a string match.

michael-s-molina · 2024-06-13T11:22:37Z

Set isolation level to READ COMMITTED

@john-bodley looking at the changed files I was not able to see where you set the isolation level to READ COMMITTED in this PR. Did you forget to commit something?

john-bodley · 2024-06-13T15:57:12Z

@michael-s-molina I updated the PR title and description.

It's actually hard to set this globally given that not all of our backends support the READ COMMITTED. I did add a comment in the config.py and updated the test configs so both MySQL and PostgreSQL will have similar behavior.

sadpandajoe · 2024-06-18T01:06:39Z

superset/config.py

+# isolation level is READ COMMITTED. All backends should use READ COMMITTED (or similar)
+# to help ensure consistent behavior.
+SQLALCHEMY_ENGINE_OPTIONS = {
+    "isolation_level": "SERIALIZABLE",  # SQLite does not support READ COMMITTED.


@john-bodley Reading this it looks like everything will be SERIALIZABLE right? we're overriding it in the tests but are we overriding it as READ COMMITTED anywhere else for the other databases?

@sadpandajoe followed the rabbit hole here, and I think you're right, this will bump all envs who don't set their SQLALCHEMY_ENGINE_OPTIONS to a higher isolation level.

How about this draft? #30174

#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

pull-request-size bot added the size/XS label May 21, 2024

john-bodley added review:checkpoint Last PR reviewed during the daily review standup review:draft and removed review:checkpoint Last PR reviewed during the daily review standup labels May 21, 2024

john-bodley force-pushed the john-bodley--mysql-read-committed branch from 4076016 to 05a93c6 Compare June 12, 2024 16:04

john-bodley changed the title ~~chore: Set isolation level as READ COMMITTED~~ chore: Set isolation level as READ COMMITTED for MySQL Jun 12, 2024

john-bodley changed the title ~~chore: Set isolation level as READ COMMITTED for MySQL~~ chore: Set isolation level as READ COMMITTED Jun 12, 2024

john-bodley changed the title ~~chore: Set isolation level as READ COMMITTED~~ chore: Set isolation level as READ COMMITTED for MySQL Jun 12, 2024

john-bodley changed the title ~~chore: Set isolation level as READ COMMITTED for MySQL~~ chore: Set isolation level Jun 12, 2024

john-bodley force-pushed the john-bodley--mysql-read-committed branch from 05a93c6 to da94f58 Compare June 12, 2024 17:58

pull-request-size bot added size/S and removed size/XS labels Jun 12, 2024

john-bodley force-pushed the john-bodley--mysql-read-committed branch from da94f58 to 8639177 Compare June 12, 2024 21:23

chore: Set isolation level as READ COMMITTED

6557097

john-bodley force-pushed the john-bodley--mysql-read-committed branch from 8639177 to 6557097 Compare June 12, 2024 22:14

john-bodley changed the title ~~chore: Set isolation level~~ chore: Set isolation level to READ COMMITTED Jun 12, 2024

john-bodley requested review from betodealmeida, dpgaspar and michael-s-molina June 12, 2024 22:41

john-bodley marked this pull request as ready for review June 12, 2024 22:41

dosubot bot added data:connect:mysql Related to MySQL data:connect:postgres Related to Postgres labels Jun 12, 2024

john-bodley commented Jun 12, 2024

View reviewed changes

john-bodley changed the title ~~chore: Set isolation level to READ COMMITTED~~ chore: Set isolation level to READ COMMITTED for testing Jun 13, 2024

john-bodley changed the title ~~chore: Set isolation level to READ COMMITTED for testing~~ chore: Set isolation level to READ COMMITTED for testing et al. Jun 13, 2024

michael-s-molina approved these changes Jun 13, 2024

View reviewed changes

john-bodley merged commit f185bbe into apache:master Jun 13, 2024
72 checks passed

john-bodley deleted the john-bodley--mysql-read-committed branch June 13, 2024 16:36

john-bodley removed the review:draft label Jun 13, 2024

sadpandajoe reviewed Jun 18, 2024

View reviewed changes

john-bodley mentioned this pull request Jun 25, 2024

chore(tests): Fix MySQL logic #29356

Merged

9 tasks

eschutho pushed a commit that referenced this pull request Jul 24, 2024

chore: Set isolation level to READ COMMITTED for testing et al. (#28628)

03ab3ed

michael-s-molina mentioned this pull request Sep 4, 2024

sql_lab.get_sql_results encountering psycopg2.errors.SerializationFailure #30064

Closed

3 tasks

mistercrunch added the risk:breaking-change Issues or PRs that will introduce breaking changes label Sep 6, 2024

mistercrunch mentioned this pull request Sep 6, 2024

fix: set default mysql isolation level to 'READ COMMITTED' #30174

Merged

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.1.0 labels Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Set isolation level to READ COMMITTED for testing et al. #28628

chore: Set isolation level to READ COMMITTED for testing et al. #28628

john-bodley commented May 21, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading

john-bodley Jun 12, 2024

michael-s-molina commented Jun 13, 2024

john-bodley commented Jun 13, 2024 •

edited

Loading

sadpandajoe Jun 18, 2024

mistercrunch Sep 6, 2024

mistercrunch Sep 6, 2024

chore: Set isolation level to READ COMMITTED for testing et al. #28628

chore: Set isolation level to READ COMMITTED for testing et al. #28628

Conversation

john-bodley commented May 21, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

john-bodley Jun 12, 2024

Choose a reason for hiding this comment

michael-s-molina commented Jun 13, 2024

john-bodley commented Jun 13, 2024 • edited Loading

sadpandajoe Jun 18, 2024

Choose a reason for hiding this comment

mistercrunch Sep 6, 2024

Choose a reason for hiding this comment

mistercrunch Sep 6, 2024

Choose a reason for hiding this comment

john-bodley commented May 21, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading

john-bodley commented Jun 13, 2024 •

edited

Loading