Skip to content

Commit

Permalink
Delete all pruned persisted info records (#39219)
Browse files Browse the repository at this point in the history
We prune persisted info records if they meet any of the following
criteria:

```clojure
(or (contains? (persisted-info/prunable-states) current-state)
    (:archived card-info)
    (not (:dataset card-info)))
```

But we only deleted the record when:

```clojure
(when (= "deletable" current-state)
  (t2/delete! PersistedInfo :id (:id persisted-info)))
```

So any records that were in a "creating" state (persist a model, but
before it first gets persisted, make it not a model, or archive the
underlying model), we constantly pruned them but never removed the
persisted info record.

Leading to task results like:

```javascript
{"success": 21, "error": 0, "skipped": 0}
```

Because 21 things were queued up for pruning, were attempted to be
pruned, but the persisted info record never removed.
  • Loading branch information
dpsutton authored and Metabase bot committed Feb 28, 2024
1 parent 07262ba commit eb02595
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 2 deletions.
3 changes: 1 addition & 2 deletions src/metabase/task/persist_refresh.clj
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,7 @@
(log/info (trs "Unpersisting model with card-id {0}" (:card_id persisted-info)))
(try
(unpersist! refresher database persisted-info)
(when (= "deletable" current-state)
(t2/delete! PersistedInfo :id (:id persisted-info)))
(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)))
Expand Down
10 changes: 10 additions & 0 deletions test/metabase/task/persist_refresh_test.clj
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,16 @@
(is (contains? queued-for-deletion (u/the-id deletable-persisted))))))
;; we manually pass in the deleteable ones to not catch others in a running instance
(#'task.persist-refresh/prune-deletables! test-refresher [deletable parchived punmodeled])
(testing "We delete persisted_info records for all of the pruned"
(let [persisted-records (t2/select :model/PersistedInfo :id [:in (map :id [parchived punmodeled deletable])])
existing (map (comp
(update-keys {parchived 'parchived
punmodeled 'punmodeled
deletable 'deletable}
:id)
:id)
persisted-records)]
(is (= [] existing))))
;; don't assert equality if there are any deletable in the app db
(doseq [deletable-persisted [deletable punmodeled parchived]]
(is (contains? @called-on (u/the-id deletable-persisted))))
Expand Down

0 comments on commit eb02595

Please sign in to comment.