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.

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

FIX: Avoid syncing user bookmarks for users that are getting destroyed #26338

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nattsw
Copy link
Contributor

@nattsw nattsw commented Mar 25, 2024

When a user is destroyed via UserDestroyer, it first gets its Bookmarks deleted, then its Posts.

In the PostDestroyer, the SyncTopicUserBookmarked job gets called to make sure orphan bookmarks are deleted if the post is a topic. However, this job is not needed when a user is getting destroyed.

This PR adds an opt that indicates to PostDestroyer that this job doesn't need to be scheduled. This PR also optimises the sync job to skip topics where there are no bookmarks.

No bookmarks

# current job
pry(main)> 200.times.sum { Benchmark.realtime { Jobs::SyncTopicUserBookmarked.new.execute(topic_id: 50) }} / 200.0
=> 0.0016186350025236607

# modified job
pry(main)> 200.times.sum { Benchmark.realtime { Jobs::SyncTopicUserBookmarked.new.execute(topic_id: 50) }} / 200.0
=> 0.00028412999818101526

Five bookmarks

# current job
pry(main)> 200.times.sum { Benchmark.realtime { Jobs::SyncTopicUserBookmarked.new.execute(topic_id: 50) }} / 200.0
=> 0.0014668599981814622

# modified job
pry(main)> 200.times.sum { Benchmark.realtime { Jobs::SyncTopicUserBookmarked.new.execute(topic_id: 50) }} / 200.0
=> 0.0008136450103484094

Copy link
Contributor

@martin-brennan martin-brennan left a comment

Choose a reason for hiding this comment

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

As discussed internally we need to collect all the destroyed topic IDs and run the SyncTopicUserBookmarked job at the end of UserDestroyer (we need to modify the job for this)

@nattsw nattsw marked this pull request as draft March 25, 2024 06:51
@nattsw nattsw marked this pull request as ready for review March 25, 2024 15:29
);

DROP TABLE tmp_sync_topic_user_bookmarks;
SET bookmarked = CASE
Copy link
Contributor

Choose a reason for hiding this comment

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

As noted internally, the perf of this worries me a bit; the tmp_sync_topic_user_bookmarks way was done because there are potentially a lot of records changing here.

DB.exec(<<~SQL, topic_id: args[:topic_id])
SELECT bookmarks.user_id, COUNT(*)
INTO TEMP TABLE tmp_sync_topic_user_bookmarks
bookmarks_exist = DB.exec(<<~SQL, topic_id: args[:topic_id]) > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a query for a value now, this should be DB.query_single . IIRC the return value of exec is based on success of the query, not the actually return value.

WHERE topic_users.user_id = tmp_sync_topic_user_bookmarks.user_id AND
topic_users.topic_id = :topic_id AND
tmp_sync_topic_user_bookmarks.count > 0;
return unless bookmarks_exist
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return unless bookmarks_exist
return if !bookmarks_exist

This is a Sam special :P

@nattsw nattsw force-pushed the skip-topic-bookmark-sync-user-destroy branch from 5e8c5ea to 0a0eda1 Compare April 3, 2024 11:54
@jjaffeux
Copy link
Contributor

@martin-brennan @nattsw do we still want to get this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants