HOLY GRAIL OF PRs UNBLOCK PARALLEL-TEST-SAFE SETTINGS!!!!!!!!!!!1 MASSIVE TEST PERFORMANCE IMPROVEMENT!!!!! #42194

camsaul · 2024-05-03T00:14:15Z

#33616 is my white whale!

Two things were blocking us from introducing a thread-safe version of mt/with-temporary-setting-values:

changing report-timezone calls driver/notify-database-updated, which in turn nukes all open connection pools for all JDBC-based databases... originally this was because Snowflake set timezone as a connection property, but it turns out this was actually not needed at all since none of our DBs set it as a connection property anymore. There is no need to nuke connection pools if connection details don't actually change at all. This is also a massive performance improvement for tests since we can actually reuse connections for more than like 2 seconds. This PR updates things so we only nuke the pools if connection details actually change or when we delete the Database (via a new method notify-database-will-be-deleted!)
Snowflake was setting week_start as a connection property which meant its connection pool got nuked every time we changed the start-of-week Setting... instead we can just set it as a session property every time we check out a connection at the same time we set Session timezone. This means there are officially no longer any settings that should affect any database's connection properties in any way and thus no setting changes can possibly lead to connection pool invalidation

So not only is this 10x faster since we can finally reuse connection pools for the entire test run, we are now finally unblocked from implementing #33616. I'll do a follow-on PR to make that thread-safe by default and respect mt/test-helpers-set-global-values! if you actually need it to make thread-unsafe changes

…han as a connection property

camsaul · 2024-05-03T00:18:38Z

modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj

-  [expr]
-  (let [report-zone (qp.timezone/report-timezone-id-if-supported :presto-jdbc (lib.metadata/database (qp.store/metadata-provider)))
+  [driver expr]
+  (let [report-zone (qp.timezone/report-timezone-id-if-supported driver (lib.metadata/database (qp.store/metadata-provider)))


unrelated but this is a big no-no because if anyone ever creates a new driver based on :presto-jdbc it will be instantly broken because their own methods for qp.timezone/report-timezone-id-if-supported would never get called. Imagine if Postgres did this, how would Redshift even work

camsaul · 2024-05-03T00:19:01Z

modules/drivers/redshift/src/metabase/driver/redshift.clj

-(defmethod sql-jdbc.execute/set-parameter [:redshift java.time.ZonedDateTime]
-  [driver ps i t]
-  (sql-jdbc.execute/set-parameter driver ps i (t/sql-timestamp (t/with-zone-same-instant t (t/zone-id "UTC")))))


this is actually unused as of a few days ago but I forgot to remove it

camsaul · 2024-05-03T00:19:36Z

modules/drivers/snowflake/test/metabase/driver/snowflake_test.clj

-                    ["2023-10-01T00:00:00+10:00" 2]
-                    ["2023-11-01T00:00:00+11:00" 1]]
-                   (mt/with-temporary-setting-values [report-timezone "Australia/Sydney"]
+            (mt/with-temporary-setting-values [report-timezone "Australia/Sydney"]


mt/with-temporary-setting-values around the is gives you more meaningful testing context when the test fails.

camsaul · 2024-05-03T00:20:21Z

src/metabase/driver/sql_jdbc/execute.clj

-(defn- get-object-of-class-thunk [^ResultSet rs, ^long i, ^Class klass]
+(defn- get-object-of-class-thunk [^ResultSet rs ^Long i ^Class klass]


Also unrelated but ^long is wrong here since I'm 99% sure we're passing around boxed Longs rather than raw 64-bit integers

replay-io · 2024-05-03T00:33:21Z

Status	Complete ↗︎
Commit	`c553f17`
Results	❌ 2 Failed ⚠️ 4 Flaky ✅ 2454 Passed

…ckout

camsaul · 2024-05-03T02:03:23Z

test/metabase/sample_data_test.clj

@@ -36,7 +36,6 @@
  {:style/indent 1}
  [[db-binding] & body]
  `(t2.with-temp/with-temp [Database db# (sample-database-db false)]
-     (sync/sync-database! db#)


Unrelated, but syncing the database was actually only necessary in one test in this namespace but we were doing it in 5 tests, so this tiny change shaves off ~8 seconds from the test suite locally in a bunch of non-parallel tests, which is amazing. On CI it probably shaves off an hour

…ckout

snoe · 2024-05-03T15:48:27Z

src/metabase/driver/sql_jdbc/connection.clj

+  [database]
+  (when-let [old-hash (get @database-id->jdbc-spec-hash (u/the-id database))]
+    (let [new-hash (jdbc-spec-hash database)]
+      (log/tracef "Maybe invalidating connection pool for %s... old hash: %s, new hash: %s"


This feels like it should log "Invalidating" / "Not invalidating" based on the when condition.

snoe · 2024-05-03T15:53:23Z

test/metabase/query_processor_test/cumulative_aggregation_test.clj

+      (let [metadata-provider (lib.tu/merged-mock-metadata-provider
+                               (lib.metadata.jvm/application-database-metadata-provider (mt/id))
+                               {:settings {:report-timezone "UTC"}})


This feels like it would be incredibly easy to forget this and write tests that work locally and fail in CI.
I don't quite follow the source of the behaviour change here. Could tests introduce a dynamic var to do this automatically in app-db-metadata-provider?

As is, it is relatively cumbersome, does this deserve a (lib.tu/metadata-provider-in-timezone mp "UTC")?

So for the most part these actually did work on CI and did work locally unless you did something like (metabase.driver/report-timezone! "US/Pacific") and then tried to run tests in your REPL. There was kind of this implicit assumption in a lot of tests that report-timezone was either unset or set to UTC in order for tests to pass.

So it's a little unrelated to the core changes in the PR but I tried to go in and fix a bunch of tests that currently fail in the REPL if you changed your report-timezone before running them. This is the case even outside this PR, I mostly went in and tried to fix all the ones I could find

We could have done this with with-temporary-setting-values everywhere (which would affect the app DB metadata provider) but since I haven't made it thread-safe yet I did it this way instead so I could keep these tests ^:parallel.

I did think about adding a helper like lib.tu/metadata-provider-in-timezone but I thought "UTC" vs {:settings {:report-timezone "UTC"}} as the argument really isn't that much shorter and it's making the behavior less obvious, with merged-mock-metadata-provider it's pretty clear I think that you're overriding the report-timezone Setting which people are already familiar with whereas metadata-provider-in-timezone seemed like more of a mystery, if you saw that you might not know what it's doing unless you went in and looked at its definition.

Also merged-mock-metadata-provider is flexible if we need to add more stuff to it like :start-of-week or other stuff

I agree tho that it is too easy to forget to set report-timezone when it's relevant to the test results. I'm trying to think what the best way to address this is going forward. I think once I do a follow-on PR to make with-temporary-setting-values thread safe then we can just add test fixtures that set report-timezone to "UTC" and start-of-week to :sunday in the namespaces that have tests where the value of these settings matter, then you don't need to do it in every individual test (but can still change it if you need to). So these changes here are more of a stopgap until we get to that point -- then I'll probably go in and clean up a bunch of this stuff

camsaul added 3 commits May 2, 2024 23:42

Have Snowflake set week_start when checking out a connection rather t…

623b3f2

…han as a connection property

Don't nuke connection pools unless we actually need to

3d04283

If one more person hardcodes a driver name in a method I will die

f74e280

metabase-bot bot assigned camsaul May 3, 2024

metabase-bot bot added the .Team/QueryProcessor :hammer_and_wrench: label May 3, 2024

camsaul changed the title ~~HOLY GRAIL OF PRs UNBLOCK MAKING SETTINGS PARALLEL-TEST SAFE!!!!!!!!!!!1 MASSIVE TEST PERFORMANCE IMPROVEMENT!!!!!~~ HOLY GRAIL OF PRs UNBLOCK PARALLEL-TEST-SAFE SETTINGS!!!!!!!!!!!1 MASSIVE TEST PERFORMANCE IMPROVEMENT!!!!! May 3, 2024

camsaul requested a review from a team May 3, 2024 00:17

Remove unused

ca87f11

camsaul commented May 3, 2024

View reviewed changes

camsaul added the no-backport Do not backport this PR to any branch label May 3, 2024

camsaul added 2 commits May 3, 2024 01:30

Merge branch 'master' into snowflake-set-week-start-on-connection-che…

669ce1a

…ckout

Test fixes 🔧

1a544fd

camsaul commented May 3, 2024

View reviewed changes

camsaul added 8 commits May 3, 2024 02:19

Hardcode presto timezone to UTC for now

831353a

Handful of test fixes 🔧

3c67d94

Test fixes 🔧

3de56f7

Parallelize a lot of tests

2646efe

This test can be parallel.

c887722

Typo

77adf12

Fix circular def

ceb7498

Merge branch 'master' into snowflake-set-week-start-on-connection-che…

c553f17

…ckout

snoe approved these changes May 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HOLY GRAIL OF PRs UNBLOCK PARALLEL-TEST-SAFE SETTINGS!!!!!!!!!!!1 MASSIVE TEST PERFORMANCE IMPROVEMENT!!!!! #42194

HOLY GRAIL OF PRs UNBLOCK PARALLEL-TEST-SAFE SETTINGS!!!!!!!!!!!1 MASSIVE TEST PERFORMANCE IMPROVEMENT!!!!! #42194

camsaul commented May 3, 2024 •

edited

camsaul May 3, 2024

camsaul May 3, 2024

camsaul May 3, 2024

camsaul May 3, 2024

replay-io bot commented May 3, 2024 •

edited

camsaul May 3, 2024

snoe May 3, 2024

snoe May 3, 2024

camsaul May 3, 2024 •

edited

camsaul May 3, 2024

		(defn- get-object-of-class-thunk [^ResultSet rs, ^long i, ^Class klass]
		(defn- get-object-of-class-thunk [^ResultSet rs ^Long i ^Class klass]

HOLY GRAIL OF PRs UNBLOCK PARALLEL-TEST-SAFE SETTINGS!!!!!!!!!!!1 MASSIVE TEST PERFORMANCE IMPROVEMENT!!!!! #42194

Are you sure you want to change the base?

HOLY GRAIL OF PRs UNBLOCK PARALLEL-TEST-SAFE SETTINGS!!!!!!!!!!!1 MASSIVE TEST PERFORMANCE IMPROVEMENT!!!!! #42194

Conversation

camsaul commented May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay-io bot commented May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camsaul May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camsaul commented May 3, 2024 •

edited

replay-io bot commented May 3, 2024 •

edited

camsaul May 3, 2024 •

edited