Skip to content
This repository has been archived by the owner on Jan 27, 2023. It is now read-only.

Performance FTW! #44

Merged
merged 6 commits into from
Jul 5, 2022
Merged

Performance FTW! #44

merged 6 commits into from
Jul 5, 2022

Conversation

mpalmer
Copy link
Contributor

@mpalmer mpalmer commented Jun 28, 2022

Use streaming_upsert for reindexing, which makes things much quicker.

@mpalmer mpalmer requested a review from a team as a code owner June 28, 2022 07:08
Copy link
Contributor

@freshtonic freshtonic left a comment

Choose a reason for hiding this comment

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

Moar perf!

LGTM, except for the build being borked which I am sure is a minor derp.

Comment on lines 168 to 172
records = find_each.lazy.map do |r|
if r.stash_id.nil?
raise "Cannot index record ID=#{r.id} without a stash ID"
end
{ id: r.stash_id, record: r.attributes }
Copy link
Contributor

Choose a reason for hiding this comment

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

This code will only work for collections that have been indexex previously. The save! call in the old code would use ActiveRecord callbacks to ensure that a stash_id was set. This approach means a stash_id will never be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, actually (heh), this approach means that the reindex will fail, because it'll raise when it it hits the record that doesn't have a stash_id. Calling #save! to set a stash ID won't work, because that'll also trigger an upsert, and that's not great because (a) an upsert-per-row is exactly what we're trying to avoid, and (b) we're in the middle of a putStream RPC just at the moment so trying to jam a put down the middle of that will end poorly.

@mpalmer mpalmer merged commit b7db2a5 into main Jul 5, 2022
@mpalmer mpalmer deleted the performance-ftw branch July 5, 2022 03:43
freshtonic pushed a commit that referenced this pull request Dec 21, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants