From 01515725fe00e5b6f8d3de87bbdfbb909cd2d377 Mon Sep 17 00:00:00 2001 From: Ilya Zhitomirskiy Date: Thu, 29 Sep 2011 18:59:04 -0700 Subject: [PATCH] WIP trim is still needed, also possible weirdness with the mock --- lib/diaspora/redis_cache.rb | 65 ++++++++++++++++-- lib/diaspora/user/querying.rb | 19 ++--- spec/lib/diaspora/redis_cache_spec.rb | 99 ++++++++++++++++++++++++--- spec/models/user/querying_spec.rb | 34 +++++---- 4 files changed, 180 insertions(+), 37 deletions(-) diff --git a/lib/diaspora/redis_cache.rb b/lib/diaspora/redis_cache.rb index b5b8d7353..cf0b87eeb 100644 --- a/lib/diaspora/redis_cache.rb +++ b/lib/diaspora/redis_cache.rb @@ -4,15 +4,24 @@ # class RedisCache - def initialize(user_id, order) - @user_id = user_id - @order = order + + SUPPORTED_CACHES = [:created_at] #['updated_at', + CACHE_LIMIT = 100 + + def initialize(user, order_field) + @user = user + @order_field = order_field.to_s self end # @return [Boolean] def cache_exists? - redis.zcard(set_key) != 0 + self.size != 0 + end + + # @return [Integer] the cardinality of the redis set + def size + redis.zcard(set_key) end def post_ids(time=Time.now, limit=15) @@ -20,6 +29,52 @@ class RedisCache post_ids[0...limit] end + # @return [RedisCache] self + def ensure_populated! + self.repopulate! unless cache_exists? + self + end + + # @return [RedisCache] self + def repopulate! + self.populate! && self.trim! + self + end + + # @return [RedisCache] self + def populate! + # user executes query and gets back hashes + sql = @user.visible_posts_sql(:limit => CACHE_LIMIT, :order => self.order) + hashes = Post.connection.select_all(sql) + + # hashes are inserted into set in a single transaction + redis.multi do + hashes.each do |h| + self.redis.zadd(set_key, h[@order_field], h["id"]) + end + end + + self + end + + # @return [RedisCache] self + def trim! + puts "cache limit #{CACHE_LIMIT}" + puts "cache size #{self.size}" + self.redis.zremrangebyrank(set_key, 0, -(CACHE_LIMIT+1)) + self + end + + # @param order [Symbol, String] + # @return [Boolean] + def self.supported_order?(order) + SUPPORTED_CACHES.include?(order.to_sym) + end + + def order + "#{@order_field} DESC" + end + protected # @return [Redis] def redis @@ -28,6 +83,6 @@ class RedisCache # @return [String] def set_key - @set_key ||= "cache_stream_#{@user_id}_#{@order}" + @set_key ||= "cache_stream_#{@user.id}_#{@order_field}" end end diff --git a/lib/diaspora/user/querying.rb b/lib/diaspora/user/querying.rb index 437bc718d..3c53b030d 100644 --- a/lib/diaspora/user/querying.rb +++ b/lib/diaspora/user/querying.rb @@ -24,11 +24,11 @@ module Diaspora def visible_post_ids(opts={}) opts = prep_opts(opts) - if AppConfig[:redis_cache] - cache = RedisCache.new(self.id, opts[:order_field]) - if cache.cache_exists? - post_ids = cache.post_ids(opts[:max_time], opts[:limit]) - end + if AppConfig[:redis_cache] && RedisCache.supported_order?(opts[:order_field]) + cache = RedisCache.new(self, opts[:order_field]) + + cache.ensure_populated! + post_ids = cache.post_ids(opts[:max_time], opts[:limit]) end if post_ids.blank? || post_ids.length < opts[:limit] @@ -40,6 +40,11 @@ module Diaspora # @return [Array] def visible_ids_from_sql(opts={}) + opts = prep_opts(opts) + Post.connection.select_values(visible_posts_sql(opts)) + end + + def visible_posts_sql(opts={}) opts = prep_opts(opts) select_clause ='DISTINCT posts.id, posts.updated_at AS updated_at, posts.created_at AS created_at' @@ -55,9 +60,7 @@ module Diaspora posts_from_others = posts_from_others.select(select_clause).order(opts[:order_with_table]).where(Post.arel_table[opts[:order_field]].lt(opts[:max_time])) posts_from_self = posts_from_self.select(select_clause).order(opts[:order_with_table]).where(Post.arel_table[opts[:order_field]].lt(opts[:max_time])) - all_posts = "(#{posts_from_others.to_sql} LIMIT #{opts[:limit]}) UNION ALL (#{posts_from_self.to_sql} LIMIT #{opts[:limit]}) ORDER BY #{opts[:order]} LIMIT #{opts[:limit]}" - - Post.connection.select_values(all_posts) + "(#{posts_from_others.to_sql} LIMIT #{opts[:limit]}) UNION ALL (#{posts_from_self.to_sql} LIMIT #{opts[:limit]}) ORDER BY #{opts[:order]} LIMIT #{opts[:limit]}" end def contact_for(person) diff --git a/spec/lib/diaspora/redis_cache_spec.rb b/spec/lib/diaspora/redis_cache_spec.rb index 6e5ed758d..f26d9a5dc 100644 --- a/spec/lib/diaspora/redis_cache_spec.rb +++ b/spec/lib/diaspora/redis_cache_spec.rb @@ -3,18 +3,20 @@ # the COPYRIGHT file. require 'spec_helper' -require 'diaspora/redis_cache' describe RedisCache do before do - @redis = MockRedis.new - @cache = RedisCache.new(bob.id, "created_at") - @cache.stub(:redis).and_return(@redis) + #@redis = MockRedis.new + @redis = Redis.new + @redis.keys.each{|p| @redis.del(p)} + + @cache = RedisCache.new(bob, :created_at) + #@cache.stub(:redis).and_return(@redis) end - it 'gets initialized with user_id and an order field' do - cache = RedisCache.new(bob.id, "updated_at") - [:@user_id, :@order].each do |var| + it 'gets initialized with user and an created_at order' do + cache = RedisCache.new(bob, :created_at) + [:@user, :@order_field].each do |var| cache.instance_variable_get(var).should_not be_blank end end @@ -22,7 +24,7 @@ describe RedisCache do describe "#cache_exists?" do it 'returns true if the sorted set exists' do timestamp = Time.now.to_i - @redis.zadd("cache_stream_#{@bob.id}_created_at", timestamp, "post_1") + @redis.zadd("cache_stream_#{bob.id}_created_at", timestamp, "post_1") @cache.cache_exists?.should be_true end @@ -38,7 +40,7 @@ describe RedisCache do @timestamp = Time.now.to_i 30.times do |n| created_time = @timestamp - n*1000 - @redis.zadd("cache_stream_#{@bob.id}_created_at", created_time, n) + @redis.zadd("cache_stream_#{bob.id}_created_at", created_time, n) @timestamps << created_time end end @@ -56,7 +58,84 @@ describe RedisCache do end end - describe "#populate" + describe "#ensure_populated!" do + it 'does nothing if the cache is populated' do + @cache.stub(:cache_exists?).and_return(true) + @cache.should_not_receive(:repopulate!) + + @cache.ensure_populated! + end + + it 'clears and poplulates if the cache is not populated' do + @cache.stub(:cache_exists?).and_return(false) + @cache.should_receive(:repopulate!) + + @cache.ensure_populated! + end + end + + describe "#repopulate!" do + it 'populates' do + @cache.stub(:trim!).and_return(true) + @cache.should_receive(:populate!).and_return(true) + @cache.repopulate! + end + + it 'trims' do + @cache.stub(:populate!).and_return(true) + @cache.should_receive(:trim!) + @cache.repopulate! + end + end + + describe "#populate!" do + it 'queries the db with the visible post sql string' do + sql = "long_sql" + order = "created_at DESC" + @cache.should_receive(:order).and_return(order) + bob.should_receive(:visible_posts_sql).with(hash_including( + :limit => 100, + :order => order)). + and_return(sql) + + Post.connection.should_receive(:select_all).with(sql).and_return([]) + + @cache.populate! + end + + it 'adds the post from the hash to the cache' + end + + describe "#trim!" do + it 'does nothing if the set is smaller than the cache limit' do + @timestamps = [] + @timestamp = Time.now.to_i + 30.times do |n| + created_time = @timestamp - n*1000 + @redis.zadd("cache_stream_#{bob.id}_created_at", created_time, n) + @timestamps << created_time + end + + post_ids = @cache.post_ids(Time.now.to_i, @cache.size) + @cache.trim! + @cache.post_ids(Time.now.to_i, @cache.size).should == post_ids + end + + it 'trims the set to the cache limit' do + @timestamps = [] + @timestamp = Time.now.to_i + 120.times do |n| + created_time = @timestamp - n*1000 + @redis.zadd("cache_stream_#{bob.id}_created_at", created_time, n) + @timestamps << created_time + end + + post_ids = 100.times.map{|n| n} + @cache.trim! + @cache.post_ids(Time.now.to_i, @cache.size).should == post_ids[0...100] + end + end + describe "#add" describe "#remove" end diff --git a/spec/models/user/querying_spec.rb b/spec/models/user/querying_spec.rb index 7ad9edd60..0cdc999b4 100644 --- a/spec/models/user/querying_spec.rb +++ b/spec/models/user/querying_spec.rb @@ -88,37 +88,43 @@ describe User do context "RedisCache" do before do AppConfig[:redis_cache] = true + @opts = {:order => "created_at DESC"} end - context 'empty cache' do - it "does not read from the cache" do - cache = mock(:cache_exists? => false) - RedisCache.stub(:new).and_return(cache) - cache.should_not_receive(:post_ids) + after do + AppConfig[:redis_cache] = nil + end - alice.visible_post_ids - end + it "gets populated with latest 100 posts" do + cache = mock(:cache_exists? => true, :ensure_populated! => mock, :post_ids => []) + RedisCache.stub(:new).and_return(cache) + cache.should_receive(:ensure_populated!) - it "is populated" do - end + alice.visible_post_ids(@opts) + end + + describe "#ensure_populated_cache" do + it 'does nothing if the cache is already populated' + it 're-populates the cache with the latest posts (in hashes)' end context 'populated cache' do before do - @cache = mock(:cache_exists? => true) + @cache = mock(:cache_exists? => true, :ensure_populated! => mock) RedisCache.stub(:new).and_return(@cache) end - it "reads from the cache" do - @cache.stub(:post_ids).and_return([1,2,3]) - alice.visible_post_ids(:limit => 3).should == [1,2,3] + it "reads from the cache" do + @cache.should_receive(:post_ids).and_return([1,2,3]) + + alice.visible_post_ids(@opts.merge({:limit => 3})).should == [1,2,3] end it "queries if maxtime is later than the last cached post" do @cache.stub(:post_ids).and_return([]) alice.should_receive(:visible_ids_from_sql) - alice.visible_post_ids + alice.visible_post_ids(@opts) end it "does not get repopulated" do