Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix mongo OOM during sync #42140

Merged
merged 2 commits into from
May 2, 2024
Merged

Fix mongo OOM during sync #42140

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e33336
Fix mongo OOM during sync
qnkhuat May 2, 2024
0201a6a
Merge branch 'master' into fix-mongo-oom
qnkhuat May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/drivers/mongo/src/metabase/driver/mongo/util.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
skip (.skip skip)
batch-size (.batchSize (int batch-size))
sort-criteria (.sort (mongo.conversion/to-document (ordered-map/ordered-map sort-criteria))))
(mapv #(mongo.conversion/from-document % opts))))
(map #(mongo.conversion/from-document % opts))))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are more places here that use mapv. My gut says we should convert them all. Should we? @lbrdnk

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll open tech debt issue tracking this.


(defn create-index
"Create index."
Expand Down
13 changes: 13 additions & 0 deletions modules/drivers/mongo/test/metabase/driver/mongo/util_test.clj
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed we don't have any behavior test here. I can add it later if we want to. but this PR should fix the escalation only.

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
(ns metabase.driver.mongo.util-test
(:require
[clojure.test :refer :all]
[metabase.driver.mongo.connection :as mongo.connection]
[metabase.driver.mongo.util :as mongo.util]
[metabase.test :as mt]))

(deftest do-find-returns-lazy-seq-test
(mt/test-driver :mongo
(testing "do-find returns a lazy seq #42133"
(mongo.connection/with-mongo-database [db (mt/db)]
(is (= clojure.lang.LazySeq
(type (mongo.util/do-find (mongo.util/collection db "venues")))))))))
Comment on lines +12 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than coding to a concrete type, and assuming that it hasn't been realized, you could instead assert that realized? is false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yea that'd be nicer.