From 5c808ee0decac075b105e1f3a36933ce38d76255 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 19 Apr 2022 14:54:43 +0200 Subject: [PATCH] Revert support from loading Home timeline from database Unfortunately, the database query could turn out very inefficient and I did not manage to find a way to improve that. Furthermore, there were still behavior inconsistencies between fetching the timeline from Redis and fetching it from Postgres. --- app/models/home_feed.rb | 48 ---------------------- spec/models/home_feed_spec.rb | 76 ++++++----------------------------- 2 files changed, 13 insertions(+), 111 deletions(-) diff --git a/app/models/home_feed.rb b/app/models/home_feed.rb index 53550b7db..d6ebb5fa6 100644 --- a/app/models/home_feed.rb +++ b/app/models/home_feed.rb @@ -9,52 +9,4 @@ class HomeFeed < Feed def regenerating? redis.exists?("account:#{@account.id}:regeneration") end - - def get(limit, max_id = nil, since_id = nil, min_id = nil) - limit = limit.to_i - max_id = max_id.to_i if max_id.present? - since_id = since_id.to_i if since_id.present? - min_id = min_id.to_i if min_id.present? - - if min_id.present? - redis_min_id = fetch_min_redis_id - return from_redis(limit, max_id, since_id, min_id) if redis_min_id && min_id >= redis_min_id - - statuses = from_database(limit, redis_min_id, since_id, min_id) - return statuses if statuses.size >= limit - - remaining_limit = limit - statuses.size - min_id = statuses.first.id unless statuses.empty? - from_redis(remaining_limit, max_id, since_id, min_id) + statuses - else - statuses = from_redis(limit, max_id, since_id, min_id) - return statuses if statuses.size >= limit - - if since_id.present? - redis_min_id = fetch_min_redis_id - return statuses if redis_min_id.present? && since_id >= redis_min_id - end - - remaining_limit = limit - statuses.size - max_id = statuses.last.id unless statuses.empty? - statuses + from_database(remaining_limit, max_id, since_id, min_id) - end - end - - protected - - def from_database(limit, max_id, since_id, min_id) - scope = Status.where(account: @account).or(Status.where(account: @account.following)) - scope = scope.where(visibility: %i(public unlisted private)) - scope - .to_a_paginated_by_id(limit, min_id: min_id, max_id: max_id, since_id: since_id) - .reject { |status| FeedManager.instance.filter?(:home, status, @account) } - .sort_by { |status| -status.id } - end - - private - - def fetch_min_redis_id - redis.zrangebyscore(key, '(0', '(+inf', limit: [0, 1], with_scores: true).first&.first&.to_i - end end diff --git a/spec/models/home_feed_spec.rb b/spec/models/home_feed_spec.rb index 20756633c..ee7a83960 100644 --- a/spec/models/home_feed_spec.rb +++ b/spec/models/home_feed_spec.rb @@ -1,72 +1,32 @@ require 'rails_helper' RSpec.describe HomeFeed, type: :model do - let(:account) { Fabricate(:account) } - let(:followed) { Fabricate(:account) } - let(:other) { Fabricate(:account) } + let(:account) { Fabricate(:account) } subject { described_class.new(account) } describe '#get' do before do - account.follow!(followed) - - Fabricate(:status, account: account, id: 1) - Fabricate(:status, account: account, id: 2) - status = Fabricate(:status, account: followed, id: 3) - Fabricate(:mention, account: account, status: status) - Fabricate(:status, account: account, id: 10) - Fabricate(:status, account: other, id: 11) - Fabricate(:status, account: followed, id: 12, visibility: :private) - Fabricate(:status, account: followed, id: 13, visibility: :direct) + Fabricate(:status, account: account, id: 1) + Fabricate(:status, account: account, id: 2) + Fabricate(:status, account: account, id: 3) + Fabricate(:status, account: account, id: 10) end context 'when feed is generated' do before do - FeedManager.instance.populate_home(account) + Redis.current.zadd( + FeedManager.instance.key(:home, account.id), + [[4, 4], [3, 3], [2, 2], [1, 1]] + ) end - it 'gets statuses with ids in the range from redis with database' do + it 'gets statuses with ids in the range from redis' do results = subject.get(3) - expect(results.map(&:id)).to eq [12, 10, 3] + expect(results.map(&:id)).to eq [3, 2] expect(results.first.attributes.keys).to eq %w(id updated_at) end - - it 'with since_id present' do - results = subject.get(3, nil, 3, nil) - expect(results.map(&:id)).to eq [12, 10] - end - - it 'with min_id present' do - results = subject.get(3, nil, nil, 0) - expect(results.map(&:id)).to eq [3, 2, 1] - end - end - - context 'when feed is only partial' do - before do - FeedManager.instance.populate_home(account) - - Redis.current.zremrangebyrank(FeedManager.instance.key(:home, account.id), 0, -2) - end - - it 'gets statuses with ids in the range from redis with database' do - results = subject.get(3) - - expect(results.map(&:id)).to eq [12, 10, 3] - expect(results.first.attributes.keys).to eq %w(id updated_at) - end - - it 'with since_id present' do - results = subject.get(3, nil, 3, nil) - expect(results.map(&:id)).to eq [12, 10] - end - - it 'with min_id present' do - results = subject.get(3, nil, nil, 0) - expect(results.map(&:id)).to eq [3, 2, 1] - end end context 'when feed is being generated' do @@ -74,20 +34,10 @@ RSpec.describe HomeFeed, type: :model do Redis.current.set("account:#{account.id}:regeneration", true) end - it 'returns from database' do + it 'returns nothing' do results = subject.get(3) - expect(results.map(&:id)).to eq [12, 10, 3] - end - - it 'with since_id present' do - results = subject.get(3, nil, 3, nil) - expect(results.map(&:id)).to eq [12, 10] - end - - it 'with min_id present' do - results = subject.get(3, nil, nil, 0) - expect(results.map(&:id)).to eq [3, 2, 1] + expect(results.map(&:id)).to eq [] end end end