github-mirror/discourse
github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-03-20 00:27:54 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

DEV: Move user count update for channels to ensure_consistency! (#22321)

Browse Source 
This fixes a longstanding TODO to move the contents of the
UpdateUserCountsForChannels job to the ensure_consistency!
method of Chat::Channel, which runs every 15 mins as part of
periodical updates.

This commit also addresses the performance issue of the original,
where we would fetch all channels and do an individual query to
get the count and update the count of each one. Now we do it all
in one query, and only publish the changed channels to the UI.
This commit is contained in:
Martin Brennan 2023-07-03 11:41:51 +10:00 committed by GitHub
parent db80a8ce79
commit f6072ba804
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 164 additions and 126 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
plugins/chat/app/jobs/scheduled/chat/update_user_counts_for_channels.rb
View File

@ -1,32 +0,0 @@
# frozen_string_literal: true
module Jobs
# TODO (martin) Move into Chat::Channel.ensure_consistency! so it
# is run with Jobs::Chat::PeriodicalUpdates
module Chat
class UpdateUserCountsForChannels < ::Jobs::Scheduled
every 1.hour
# FIXME: This could become huge as the amount of channels grows, we
# need a different approach here. Perhaps we should only bother for
# channels updated or with new messages in the past N days? Perhaps
# we could update all the counts in a single query as well?
def execute(args = {})
return if !SiteSetting.chat_enabled
::Chat::Channel
.where(status: %i[open closed])
.find_each { |chat_channel| set_user_count(chat_channel) }
end
def set_user_count(chat_channel)
current_count = chat_channel.user_count || 0
new_count = ::Chat::ChannelMembershipsQuery.count(chat_channel)
return if current_count == new_count
chat_channel.update(user_count: new_count, user_count_stale: false)
::Chat::Publisher.publish_chat_channel_metadata(chat_channel)
end
end
end
end

56
plugins/chat/app/models/chat/channel.rb
View File

@ -104,27 +104,53 @@ module Chat
end
def self.ensure_consistency!
update_counts
update_message_counts
update_user_counts
end
# TODO (martin) Move Jobs::Chat::UpdateUserCountsForChannels into here
def self.update_counts
def self.update_message_counts
# NOTE: Chat::Channel#messages_count is not updated every time
# a message is created or deleted in a channel, so it should not
# be displayed in the UI. It is updated eventually via Jobs::Chat::PeriodicalUpdates
DB.exec <<~SQL
UPDATE chat_channels channels
SET messages_count = subquery.messages_count
FROM (
SELECT COUNT(*) AS messages_count, chat_channel_id
FROM chat_messages
WHERE chat_messages.deleted_at IS NULL
GROUP BY chat_channel_id
) subquery
WHERE channels.id = subquery.chat_channel_id
AND channels.deleted_at IS NULL
AND subquery.messages_count != channels.messages_count
SQL
UPDATE chat_channels channels
SET messages_count = subquery.messages_count
FROM (
SELECT COUNT(*) AS messages_count, chat_channel_id
FROM chat_messages
WHERE chat_messages.deleted_at IS NULL
GROUP BY chat_channel_id
) subquery
WHERE channels.id = subquery.chat_channel_id
AND channels.deleted_at IS NULL
AND subquery.messages_count != channels.messages_count
SQL
end
def self.update_user_counts
updated_channel_ids = DB.query_single(<<~SQL, statuses: [statuses[:open], statuses[:closed]])
UPDATE chat_channels channels
SET user_count = subquery.user_count, user_count_stale = false
FROM (
SELECT COUNT(DISTINCT user_chat_channel_memberships.id) AS user_count, user_chat_channel_memberships.chat_channel_id
FROM user_chat_channel_memberships
INNER JOIN users ON users.id = user_chat_channel_memberships.user_id
WHERE users.active
AND (users.suspended_till IS NULL OR users.suspended_till <= CURRENT_TIMESTAMP)
AND NOT users.staged
AND user_chat_channel_memberships.following
GROUP BY user_chat_channel_memberships.chat_channel_id
) subquery
WHERE channels.id = subquery.chat_channel_id
AND channels.deleted_at IS NULL
AND subquery.user_count != channels.user_count
AND channels.status IN (:statuses)
RETURNING channels.id;
SQL
Chat::Channel
.where(id: updated_channel_ids)
.find_each { |channel| ::Chat::Publisher.publish_chat_channel_metadata(channel) }
end
def latest_not_deleted_message_id(anchor_message_id: nil)

59
plugins/chat/spec/jobs/scheduled/update_user_counts_for_channels_spec.rb
View File

@ -1,59 +0,0 @@
# frozen_string_literal: true
require "rails_helper"
describe Jobs::Chat::UpdateUserCountsForChannels do
fab!(:chat_channel_1) { Fabricate(:category_channel, user_count: 0) }
fab!(:chat_channel_2) { Fabricate(:category_channel, user_count: 0) }
fab!(:user_1) { Fabricate(:user) }
fab!(:user_2) { Fabricate(:user) }
fab!(:user_3) { Fabricate(:user) }
fab!(:user_4) { Fabricate(:user) }
def create_memberships
user_1.user_chat_channel_memberships.create!(chat_channel: chat_channel_1, following: true)
user_1.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true)
user_2.user_chat_channel_memberships.create!(chat_channel: chat_channel_1, following: true)
user_2.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true)
user_3.user_chat_channel_memberships.create!(chat_channel: chat_channel_1, following: false)
user_3.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true)
end
it "sets the user_count correctly for each chat channel" do
create_memberships
Jobs::Chat::UpdateUserCountsForChannels.new.execute
expect(chat_channel_1.reload.user_count).to eq(2)
expect(chat_channel_2.reload.user_count).to eq(3)
end
it "does not count suspended, non-activated, nor staged users" do
user_1.user_chat_channel_memberships.create!(chat_channel: chat_channel_1, following: true)
user_2.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true)
user_3.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true)
user_4.user_chat_channel_memberships.create!(chat_channel: chat_channel_2, following: true)
user_2.update(suspended_till: 3.weeks.from_now)
user_3.update(staged: true)
user_4.update(active: false)
Jobs::Chat::UpdateUserCountsForChannels.new.execute
expect(chat_channel_1.reload.user_count).to eq(1)
expect(chat_channel_2.reload.user_count).to eq(0)
end
it "does not count archived, or read_only channels" do
create_memberships
chat_channel_1.update!(status: :archived)
Jobs::Chat::UpdateUserCountsForChannels.new.execute
expect(chat_channel_1.reload.user_count).to eq(0)
chat_channel_1.update!(status: :read_only)
Jobs::Chat::UpdateUserCountsForChannels.new.execute
expect(chat_channel_1.reload.user_count).to eq(0)
end
end

141
plugins/chat/spec/models/chat/channel_spec.rb
View File

@ -25,27 +25,28 @@ RSpec.describe Chat::Channel do
describe ".ensure_consistency!" do
fab!(:category_channel2) { Fabricate(:category_channel) }
fab!(:category_channel3) { Fabricate(:category_channel) }
fab!(:category_channel4) { Fabricate(:category_channel) }
fab!(:dm_channel2) { Fabricate(:direct_message_channel) }
before do
Fabricate(:chat_message, chat_channel: category_channel1)
Fabricate(:chat_message, chat_channel: category_channel1)
Fabricate(:chat_message, chat_channel: category_channel1)
Fabricate(:chat_message, chat_channel: category_channel2)
Fabricate(:chat_message, chat_channel: category_channel2)
Fabricate(:chat_message, chat_channel: category_channel2)
Fabricate(:chat_message, chat_channel: category_channel2)
Fabricate(:chat_message, chat_channel: category_channel3)
Fabricate(:chat_message, chat_channel: dm_channel2)
Fabricate(:chat_message, chat_channel: dm_channel2)
end
describe "updating messages_count for all channels" do
fab!(:category_channel3) { Fabricate(:category_channel) }
fab!(:category_channel4) { Fabricate(:category_channel) }
fab!(:dm_channel2) { Fabricate(:direct_message_channel) }
before do
Fabricate(:chat_message, chat_channel: category_channel1)
Fabricate(:chat_message, chat_channel: category_channel1)
Fabricate(:chat_message, chat_channel: category_channel1)
Fabricate(:chat_message, chat_channel: category_channel2)
Fabricate(:chat_message, chat_channel: category_channel2)
Fabricate(:chat_message, chat_channel: category_channel2)
Fabricate(:chat_message, chat_channel: category_channel2)
Fabricate(:chat_message, chat_channel: category_channel3)
Fabricate(:chat_message, chat_channel: dm_channel2)
Fabricate(:chat_message, chat_channel: dm_channel2)
end
it "counts correctly" do
described_class.ensure_consistency!
expect(category_channel1.reload.messages_count).to eq(3)
@ -70,6 +71,108 @@ RSpec.describe Chat::Channel do
expect(category_channel3.reload.messages_count).to eq(1)
end
end
describe "updating user_count for all channels" do
fab!(:user_1) { Fabricate(:user) }
fab!(:user_2) { Fabricate(:user) }
fab!(:user_3) { Fabricate(:user) }
fab!(:user_4) { Fabricate(:user) }
def create_memberships
user_1.user_chat_channel_memberships.create!(
chat_channel: category_channel1,
following: true,
)
user_1.user_chat_channel_memberships.create!(
chat_channel: category_channel2,
following: true,
)
user_2.user_chat_channel_memberships.create!(
chat_channel: category_channel1,
following: true,
)
user_2.user_chat_channel_memberships.create!(
chat_channel: category_channel2,
following: true,
)
user_3.user_chat_channel_memberships.create!(
chat_channel: category_channel1,
following: false,
)
user_3.user_chat_channel_memberships.create!(
chat_channel: category_channel2,
following: true,
)
end
it "sets the user_count correctly for each chat channel" do
create_memberships
described_class.ensure_consistency!
expect(category_channel1.reload.user_count).to eq(2)
expect(category_channel2.reload.user_count).to eq(3)
end
it "does not count suspended, non-activated, nor staged users" do
user_1.user_chat_channel_memberships.create!(
chat_channel: category_channel1,
following: true,
)
user_2.user_chat_channel_memberships.create!(
chat_channel: category_channel2,
following: true,
)
user_3.user_chat_channel_memberships.create!(
chat_channel: category_channel2,
following: true,
)
user_4.user_chat_channel_memberships.create!(
chat_channel: category_channel2,
following: true,
)
user_2.update(suspended_till: 3.weeks.from_now)
user_3.update(staged: true)
user_4.update(active: false)
described_class.ensure_consistency!
expect(category_channel1.reload.user_count).to eq(1)
expect(category_channel2.reload.user_count).to eq(0)
end
it "does not count archived, or read_only channels" do
create_memberships
category_channel1.update!(status: :archived)
described_class.ensure_consistency!
expect(category_channel1.reload.user_count).to eq(0)
category_channel1.update!(status: :read_only)
described_class.ensure_consistency!
expect(category_channel1.reload.user_count).to eq(0)
end
it "publishes all the updated channels" do
create_memberships
messages = MessageBus.track_publish { described_class.ensure_consistency! }
expect(messages.length).to eq(3)
expect(messages.map(&:data)).to match_array(
[
{ chat_channel_id: category_channel1.id, memberships_count: 2 },
{ chat_channel_id: category_channel2.id, memberships_count: 3 },
{ chat_channel_id: dm_channel1.id, memberships_count: 2 },
],
)
messages = MessageBus.track_publish { described_class.ensure_consistency! }
expect(messages.length).to eq(0)
end
end
end
describe "#allow_channel_wide_mentions" do

2
plugins/chat/spec/plugin_spec.rb
View File

@ -170,7 +170,7 @@ describe Chat do
user_2.user_chat_channel_memberships.create!(chat_channel: chat_channel, following: true)
user_3.user_chat_channel_memberships.create!(chat_channel: chat_channel, following: true)
user_4.user_chat_channel_memberships.create!(chat_channel: chat_channel, following: true)
Jobs::Chat::UpdateUserCountsForChannels.new.execute({})
Chat::Channel.ensure_consistency!
expect(Oneboxer.preview(chat_url)).to match_html <<~HTML
<aside class="onebox chat-onebox">