Skip to content
Snippets Groups Projects
Unverified Commit 80f90f4f authored by dpsutton's avatar dpsutton Committed by GitHub
Browse files

Don't delete "off" persisted_info records (#39272)

* Don't delete "off" persisted_info records

If they are off, they are "eligible" models for persistence that we do
not want to persist. And to remember that use choice to not persist them
we need to keep the record around. This was handled previously, but
https://github.com/metabase/metabase/pull/39219 was a bit too blunt. We
had a bunch of records in a "creating" state that we weren't removing,
so i deleted them regardless of state. But we want to make sure that the
"off" ones are not deleted.

On stats, a bunch of ones that were set to off are now slated to be
persisted after the prune job deleted the persisted_info records.

* unused local. whoops
parent 9594057e
Branches
Tags
No related merge requests found
(ns metabase-enterprise.advanced-config.caching-test
(:require
[clojure.set :as set]
[clojure.test :refer :all]
[java-time.api :as t]
[metabase.models :refer [Card Dashboard Database PersistedInfo TaskHistory]]
......@@ -74,9 +75,9 @@
(mt/with-model-cleanup [TaskHistory]
(testing "with :cache-granular-controls enabled, don't refresh any tables in an 'off' or 'deletable' state"
(mt/with-premium-features #{:cache-granular-controls}
(with-temp-persist-models [db creating]
(with-temp-persist-models [db creating poff pdeletable]
(testing "Calls refresh on each persisted-info row"
(let [card-ids (atom #{})
(let [card-ids (atom #{})
test-refresher (reify task.persist-refresh/Refresher
(refresh! [_ _database _definition card]
(swap! card-ids conj (:id card))
......@@ -85,12 +86,25 @@
(#'task.persist-refresh/refresh-tables! (u/the-id db) test-refresher)
(testing "Doesn't refresh models that have state='off' or 'deletable' if :cache-granular-controls feature flag is enabled"
(is (= #{(u/the-id creating)} @card-ids)))
(is (partial= {:task "persist-refresh"
(is (partial= {:task "persist-refresh"
:task_details {:success 1 :error 0}}
(t2/select-one TaskHistory
:db_id (u/the-id db)
:task "persist-refresh"
{:order-by [[:id :desc]]}))))))))))
{:order-by [[:id :desc]]})))
(testing "Deletes backing tables of models that have state='off'"
(let [unpersisted-ids (atom #{})
test-refresher (reify task.persist-refresh/Refresher
(unpersist! [_ _database persisted-info]
(swap! unpersisted-ids conj (:id persisted-info))))
deleted? (fn [{id :id}]
(not (t2/exists? :model/PersistedInfo :id id)))]
(#'task.persist-refresh/prune-all-deletable! test-refresher)
(is (set/subset? (set [(:id pdeletable) (:id poff)])
@unpersisted-ids))
(is (deleted? pdeletable))
(testing "But does not delete the persisted_info record for \"off\" models"
(is (not (deleted? poff)))))))))))))
(deftest model-caching-granular-controls-test-2
(mt/with-model-cleanup [TaskHistory]
......
......@@ -133,7 +133,8 @@
(log/info (trs "Unpersisting model with card-id {0}" (:card_id persisted-info)))
(try
(unpersist! refresher database persisted-info)
(t2/delete! PersistedInfo :id (:id persisted-info))
(when-not (= "off" current-state)
(t2/delete! PersistedInfo :id (:id persisted-info)))
(update stats :success inc)
(catch Exception e
(log/info e (trs "Error unpersisting model with card-id {0}" (:card_id persisted-info)))
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment