From c62fce988308e545488cbb32569f022e771aa799 Mon Sep 17 00:00:00 2001 From: Imre Farkas Date: Thu, 12 Jul 2018 13:21:08 +0200 Subject: Delete UserActivities and related workers --- app/services/users/activity_service.rb | 20 +++- app/workers/all_queues.yml | 2 - .../schedule_update_user_activity_worker.rb | 12 -- app/workers/update_user_activity_worker.rb | 27 ----- .../43312-remove_user_activity_workers.yml | 5 + config/initializers/1_settings.rb | 4 - config/sidekiq_queues.yml | 2 - lib/gitlab/user_activities.rb | 34 ------ spec/controllers/sessions_controller_spec.rb | 4 +- spec/lib/gitlab/user_activities_spec.rb | 127 --------------------- spec/requests/api/internal_spec.rb | 16 +-- spec/requests/git_http_spec.rb | 5 +- spec/services/event_create_service_spec.rb | 4 +- spec/services/users/activity_service_spec.rb | 65 +++++------ spec/support/helpers/user_activities_helpers.rb | 7 -- spec/support/matchers/user_activity_matchers.rb | 5 - .../schedule_update_user_activity_worker_spec.rb | 25 ---- spec/workers/update_user_activity_worker_spec.rb | 35 ------ 18 files changed, 66 insertions(+), 333 deletions(-) delete mode 100644 app/workers/schedule_update_user_activity_worker.rb delete mode 100644 app/workers/update_user_activity_worker.rb create mode 100644 changelogs/unreleased/43312-remove_user_activity_workers.yml delete mode 100644 lib/gitlab/user_activities.rb delete mode 100644 spec/lib/gitlab/user_activities_spec.rb delete mode 100644 spec/support/helpers/user_activities_helpers.rb delete mode 100644 spec/support/matchers/user_activity_matchers.rb delete mode 100644 spec/workers/schedule_update_user_activity_worker_spec.rb delete mode 100644 spec/workers/update_user_activity_worker_spec.rb diff --git a/app/services/users/activity_service.rb b/app/services/users/activity_service.rb index 5803404c3c8..ffb19ea4267 100644 --- a/app/services/users/activity_service.rb +++ b/app/services/users/activity_service.rb @@ -1,12 +1,19 @@ module Users class ActivityService + LEASE_TIMEOUT = 1.minute.to_i + def initialize(author, activity) - @author = author.respond_to?(:user) ? author.user : author + @user = if author.respond_to?(:username) + author + elsif author.respond_to?(:user) + author.user + end + @activity = activity end def execute - return unless @author && @author.is_a?(User) + return unless @user record_activity end @@ -14,9 +21,14 @@ module Users private def record_activity - Gitlab::UserActivities.record(@author.id) if Gitlab::Database.read_write? + return if Gitlab::Database.read_only? + + lease = Gitlab::ExclusiveLease.new("acitvity_service:#{@user.id}", + timeout: LEASE_TIMEOUT) + return unless lease.try_obtain - Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@author.id} (username: #{@author.username})") + @user.update_attribute(:last_activity_on, Date.today) + Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@user.id} (username: #{@user.username})") end end end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index d4be1ccfcfa..4de35b9bd06 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -13,7 +13,6 @@ - cronjob:repository_archive_cache - cronjob:repository_check_dispatch - cronjob:requests_profiles -- cronjob:schedule_update_user_activity - cronjob:stuck_ci_jobs - cronjob:stuck_import_jobs - cronjob:stuck_merge_jobs @@ -114,7 +113,6 @@ - storage_migrator - system_hook_push - update_merge_requests -- update_user_activity - upload_checksum - web_hook - repository_update_remote_mirror diff --git a/app/workers/schedule_update_user_activity_worker.rb b/app/workers/schedule_update_user_activity_worker.rb deleted file mode 100644 index ff42fb8f0e5..00000000000 --- a/app/workers/schedule_update_user_activity_worker.rb +++ /dev/null @@ -1,12 +0,0 @@ -# frozen_string_literal: true - -class ScheduleUpdateUserActivityWorker - include ApplicationWorker - include CronjobQueue - - def perform(batch_size = 500) - Gitlab::UserActivities.new.each_slice(batch_size) do |batch| - UpdateUserActivityWorker.perform_async(Hash[batch]) - end - end -end diff --git a/app/workers/update_user_activity_worker.rb b/app/workers/update_user_activity_worker.rb deleted file mode 100644 index 15f01a70337..00000000000 --- a/app/workers/update_user_activity_worker.rb +++ /dev/null @@ -1,27 +0,0 @@ -# frozen_string_literal: true - -class UpdateUserActivityWorker - include ApplicationWorker - - def perform(pairs) - pairs = cast_data(pairs) - ids = pairs.keys - conditions = 'WHEN id = ? THEN ? ' * ids.length - - User.where(id: ids) - .update_all([ - "last_activity_on = CASE #{conditions} ELSE last_activity_on END", - *pairs.to_a.flatten - ]) - - Gitlab::UserActivities.new.delete(*ids) - end - - private - - def cast_data(pairs) - pairs.each_with_object({}) do |(key, value), new_pairs| - new_pairs[key.to_i] = Time.at(value.to_i).to_s(:db) - end - end -end diff --git a/changelogs/unreleased/43312-remove_user_activity_workers.yml b/changelogs/unreleased/43312-remove_user_activity_workers.yml new file mode 100644 index 00000000000..6dfd018e350 --- /dev/null +++ b/changelogs/unreleased/43312-remove_user_activity_workers.yml @@ -0,0 +1,5 @@ +--- +title: Delete UserActivities and related workers +merge_request: 20597 +author: +type: performance diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 4b9cc59ec45..b4868596785 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -318,10 +318,6 @@ Settings.cron_jobs['gitlab_usage_ping_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['gitlab_usage_ping_worker']['cron'] ||= Settings.__send__(:cron_for_usage_ping) Settings.cron_jobs['gitlab_usage_ping_worker']['job_class'] = 'GitlabUsagePingWorker' -Settings.cron_jobs['schedule_update_user_activity_worker'] ||= Settingslogic.new({}) -Settings.cron_jobs['schedule_update_user_activity_worker']['cron'] ||= '30 0 * * *' -Settings.cron_jobs['schedule_update_user_activity_worker']['job_class'] = 'ScheduleUpdateUserActivityWorker' - Settings.cron_jobs['remove_old_web_hook_logs_worker'] ||= Settingslogic.new({}) Settings.cron_jobs['remove_old_web_hook_logs_worker']['cron'] ||= '40 0 * * *' Settings.cron_jobs['remove_old_web_hook_logs_worker']['job_class'] = 'RemoveOldWebHookLogsWorker' diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 3400142db36..70b584ff9e9 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -62,7 +62,6 @@ - [default, 1] - [pages, 1] - [system_hook_push, 1] - - [update_user_activity, 1] - [propagate_service_template, 1] - [background_migration, 1] - [gcp_cluster, 1] @@ -77,4 +76,3 @@ - [repository_remove_remote, 1] - [create_note_diff_file, 1] - [delete_diff_files, 1] - diff --git a/lib/gitlab/user_activities.rb b/lib/gitlab/user_activities.rb deleted file mode 100644 index 125488536e1..00000000000 --- a/lib/gitlab/user_activities.rb +++ /dev/null @@ -1,34 +0,0 @@ -module Gitlab - class UserActivities - include Enumerable - - KEY = 'users:activities'.freeze - BATCH_SIZE = 500 - - def self.record(key, time = Time.now) - Gitlab::Redis::SharedState.with do |redis| - redis.hset(KEY, key, time.to_i) - end - end - - def delete(*keys) - Gitlab::Redis::SharedState.with do |redis| - redis.hdel(KEY, keys) - end - end - - def each - cursor = 0 - loop do - cursor, pairs = - Gitlab::Redis::SharedState.with do |redis| - redis.hscan(KEY, cursor, count: BATCH_SIZE) - end - - Hash[pairs].each { |pair| yield pair } - - break if cursor == '0' - end - end - end -end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 7c00652317b..8e25b61e2f1 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -50,8 +50,6 @@ describe SessionsController do end context 'when using valid password', :clean_gitlab_redis_shared_state do - include UserActivitiesHelpers - let(:user) { create(:user) } let(:user_params) { { login: user.username, password: user.password } } @@ -77,7 +75,7 @@ describe SessionsController do it 'updates the user activity' do expect do post(:create, user: user_params) - end.to change { user_activity(user) } + end.to change { user.reload.last_activity_on }.to(Date.today) end end diff --git a/spec/lib/gitlab/user_activities_spec.rb b/spec/lib/gitlab/user_activities_spec.rb deleted file mode 100644 index 6bce2ee13cf..00000000000 --- a/spec/lib/gitlab/user_activities_spec.rb +++ /dev/null @@ -1,127 +0,0 @@ -require 'spec_helper' - -describe Gitlab::UserActivities, :clean_gitlab_redis_shared_state do - let(:now) { Time.now } - - describe '.record' do - context 'with no time given' do - it 'uses Time.now and records an activity in SharedState' do - Timecop.freeze do - now # eager-load now - described_class.record(42) - end - - Gitlab::Redis::SharedState.with do |redis| - expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]]) - end - end - end - - context 'with a time given' do - it 'uses the given time and records an activity in SharedState' do - described_class.record(42, now) - - Gitlab::Redis::SharedState.with do |redis| - expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]]) - end - end - end - end - - describe '.delete' do - context 'with a single key' do - context 'and key exists' do - it 'removes the pair from SharedState' do - described_class.record(42, now) - - Gitlab::Redis::SharedState.with do |redis| - expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]]) - end - - subject.delete(42) - - Gitlab::Redis::SharedState.with do |redis| - expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []]) - end - end - end - - context 'and key does not exist' do - it 'removes the pair from SharedState' do - Gitlab::Redis::SharedState.with do |redis| - expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []]) - end - - subject.delete(42) - - Gitlab::Redis::SharedState.with do |redis| - expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []]) - end - end - end - end - - context 'with multiple keys' do - context 'and all keys exist' do - it 'removes the pair from SharedState' do - described_class.record(41, now) - described_class.record(42, now) - - Gitlab::Redis::SharedState.with do |redis| - expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['41', now.to_i.to_s], ['42', now.to_i.to_s]]]) - end - - subject.delete(41, 42) - - Gitlab::Redis::SharedState.with do |redis| - expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []]) - end - end - end - - context 'and some keys does not exist' do - it 'removes the existing pair from SharedState' do - described_class.record(42, now) - - Gitlab::Redis::SharedState.with do |redis| - expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]]) - end - - subject.delete(41, 42) - - Gitlab::Redis::SharedState.with do |redis| - expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []]) - end - end - end - end - end - - describe 'Enumerable' do - before do - described_class.record(40, now) - described_class.record(41, now) - described_class.record(42, now) - end - - it 'allows to read the activities sequentially' do - expected = { '40' => now.to_i.to_s, '41' => now.to_i.to_s, '42' => now.to_i.to_s } - - actual = described_class.new.each_with_object({}) do |(key, time), actual| - actual[key] = time - end - - expect(actual).to eq(expected) - end - - context 'with many records' do - before do - 1_000.times { |i| described_class.record(i, now) } - end - - it 'is possible to loop through all the records' do - expect(described_class.new.count).to eq(1_000) - end - end - end -end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index a56b913198c..a2cfa706f58 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -279,7 +279,7 @@ describe API::Internal do expect(json_response["status"]).to be_truthy expect(json_response["repository_path"]).to eq('/') expect(json_response["gl_repository"]).to eq("wiki-#{project.id}") - expect(user).not_to have_an_activity_record + expect(user.reload.last_activity_on).to be_nil end end @@ -291,7 +291,7 @@ describe API::Internal do expect(json_response["status"]).to be_truthy expect(json_response["repository_path"]).to eq('/') expect(json_response["gl_repository"]).to eq("wiki-#{project.id}") - expect(user).to have_an_activity_record + expect(user.reload.last_activity_on).to eql(Date.today) end end @@ -309,7 +309,7 @@ describe API::Internal do expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path) expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage)) expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage)) - expect(user).to have_an_activity_record + expect(user.reload.last_activity_on).to eql(Date.today) end end @@ -328,7 +328,7 @@ describe API::Internal do expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path) expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage)) expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage)) - expect(user).not_to have_an_activity_record + expect(user.reload.last_activity_on).to be_nil end end end @@ -345,7 +345,7 @@ describe API::Internal do expect(response).to have_gitlab_http_status(200) expect(json_response["status"]).to be_falsey - expect(user).not_to have_an_activity_record + expect(user.reload.last_activity_on).to be_nil end end @@ -355,7 +355,7 @@ describe API::Internal do expect(response).to have_gitlab_http_status(200) expect(json_response["status"]).to be_falsey - expect(user).not_to have_an_activity_record + expect(user.reload.last_activity_on).to be_nil end end end @@ -373,7 +373,7 @@ describe API::Internal do expect(response).to have_gitlab_http_status(200) expect(json_response["status"]).to be_falsey - expect(user).not_to have_an_activity_record + expect(user.reload.last_activity_on).to be_nil end end @@ -383,7 +383,7 @@ describe API::Internal do expect(response).to have_gitlab_http_status(200) expect(json_response["status"]).to be_falsey - expect(user).not_to have_an_activity_record + expect(user.reload.last_activity_on).to be_nil end end end diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index b030d9862c6..0f3e7157e14 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -5,7 +5,6 @@ describe 'Git HTTP requests' do include TermsHelper include GitHttpHelpers include WorkhorseHelpers - include UserActivitiesHelpers shared_examples 'pulls require Basic HTTP Authentication' do context "when no credentials are provided" do @@ -440,10 +439,10 @@ describe 'Git HTTP requests' do end it 'updates the user last activity', :clean_gitlab_redis_shared_state do - expect(user_activity(user)).to be_nil + expect(user.last_activity_on).to be_nil download(path, env) do |response| - expect(user_activity(user)).to be_present + expect(user.reload.last_activity_on).to eql(Date.today) end end end diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 13395a7cac3..68e310b0506 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -1,8 +1,6 @@ require 'spec_helper' describe EventCreateService do - include UserActivitiesHelpers - let(:service) { described_class.new } describe 'Issues' do @@ -146,7 +144,7 @@ describe EventCreateService do it 'updates user last activity' do expect { service.push(project, user, push_data) } - .to change { user_activity(user) } + .to change { user.last_activity_on }.to(Date.today) end it 'caches the last push event for the user' do diff --git a/spec/services/users/activity_service_spec.rb b/spec/services/users/activity_service_spec.rb index 17eabad73be..f20849e6924 100644 --- a/spec/services/users/activity_service_spec.rb +++ b/spec/services/users/activity_service_spec.rb @@ -1,60 +1,61 @@ require 'spec_helper' describe Users::ActivityService do - include UserActivitiesHelpers + include ExclusiveLeaseHelpers - let(:user) { create(:user) } + let(:user) { create(:user, last_activity_on: last_activity_on) } - subject(:service) { described_class.new(user, 'type') } + subject { described_class.new(user, 'type') } describe '#execute', :clean_gitlab_redis_shared_state do context 'when last activity is nil' do - before do - service.execute - end + let(:last_activity_on) { nil } - it 'sets the last activity timestamp for the user' do - expect(last_hour_user_ids).to eq([user.id]) + it 'updates last_activity_on for the user' do + expect { subject.execute } + .to change(user, :last_activity_on).from(last_activity_on).to(Date.today) end + end - it 'updates the same user' do - service.execute + context 'when last activity is in the past' do + let(:last_activity_on) { Date.today - 1.week } - expect(last_hour_user_ids).to eq([user.id]) - end - - it 'updates the timestamp of an existing user' do - Timecop.freeze(Date.tomorrow) do - expect { service.execute }.to change { user_activity(user) }.to(Time.now.to_i.to_s) - end + it 'updates last_activity_on for the user' do + expect { subject.execute } + .to change(user, :last_activity_on) + .from(last_activity_on) + .to(Date.today) end + end - describe 'other user' do - it 'updates other user' do - other_user = create(:user) - described_class.new(other_user, 'type').execute + context 'when last activity is today' do + let(:last_activity_on) { Date.today } - expect(last_hour_user_ids).to match_array([user.id, other_user.id]) - end + it 'does not update last_activity_on' do + expect { subject.execute }.not_to change(user, :last_activity_on) end end context 'when in GitLab read-only instance' do + let(:last_activity_on) { nil } + before do allow(Gitlab::Database).to receive(:read_only?).and_return(true) end - it 'does not update last_activity_at' do - service.execute - - expect(last_hour_user_ids).to eq([]) + it 'does not update last_activity_on' do + expect { subject.execute }.not_to change(user, :last_activity_on) end end - end - def last_hour_user_ids - Gitlab::UserActivities.new - .select { |k, v| v >= 1.hour.ago.to_i.to_s } - .map { |k, _| k.to_i } + context 'when a lease could not be obtained' do + let(:last_activity_on) { nil } + + it 'does not update last_activity_on' do + stub_exclusive_lease_taken("acitvity_service:#{user.id}", timeout: 1.minute.to_i) + + expect { subject.execute }.not_to change(user, :last_activity_on) + end + end end end diff --git a/spec/support/helpers/user_activities_helpers.rb b/spec/support/helpers/user_activities_helpers.rb deleted file mode 100644 index 44feb104644..00000000000 --- a/spec/support/helpers/user_activities_helpers.rb +++ /dev/null @@ -1,7 +0,0 @@ -module UserActivitiesHelpers - def user_activity(user) - Gitlab::UserActivities.new - .find { |k, _| k == user.id.to_s }&. - second - end -end diff --git a/spec/support/matchers/user_activity_matchers.rb b/spec/support/matchers/user_activity_matchers.rb deleted file mode 100644 index ce3b683b6d2..00000000000 --- a/spec/support/matchers/user_activity_matchers.rb +++ /dev/null @@ -1,5 +0,0 @@ -RSpec::Matchers.define :have_an_activity_record do |expected| - match do |user| - expect(Gitlab::UserActivities.new.find { |k, _| k == user.id.to_s }).to be_present - end -end diff --git a/spec/workers/schedule_update_user_activity_worker_spec.rb b/spec/workers/schedule_update_user_activity_worker_spec.rb deleted file mode 100644 index 32c59381b01..00000000000 --- a/spec/workers/schedule_update_user_activity_worker_spec.rb +++ /dev/null @@ -1,25 +0,0 @@ -require 'spec_helper' - -describe ScheduleUpdateUserActivityWorker, :clean_gitlab_redis_shared_state do - let(:now) { Time.now } - - before do - Gitlab::UserActivities.record('1', now) - Gitlab::UserActivities.record('2', now) - end - - it 'schedules UpdateUserActivityWorker once' do - expect(UpdateUserActivityWorker).to receive(:perform_async).with({ '1' => now.to_i.to_s, '2' => now.to_i.to_s }) - - subject.perform - end - - context 'when specifying a batch size' do - it 'schedules UpdateUserActivityWorker twice' do - expect(UpdateUserActivityWorker).to receive(:perform_async).with({ '1' => now.to_i.to_s }) - expect(UpdateUserActivityWorker).to receive(:perform_async).with({ '2' => now.to_i.to_s }) - - subject.perform(1) - end - end -end diff --git a/spec/workers/update_user_activity_worker_spec.rb b/spec/workers/update_user_activity_worker_spec.rb deleted file mode 100644 index 268ca1d81f2..00000000000 --- a/spec/workers/update_user_activity_worker_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -require 'spec_helper' - -describe UpdateUserActivityWorker, :clean_gitlab_redis_shared_state do - let(:user_active_2_days_ago) { create(:user, current_sign_in_at: 10.months.ago) } - let(:user_active_yesterday_1) { create(:user) } - let(:user_active_yesterday_2) { create(:user) } - let(:user_active_today) { create(:user) } - let(:data) do - { - user_active_2_days_ago.id.to_s => 2.days.ago.at_midday.to_i.to_s, - user_active_yesterday_1.id.to_s => 1.day.ago.at_midday.to_i.to_s, - user_active_yesterday_2.id.to_s => 1.day.ago.at_midday.to_i.to_s, - user_active_today.id.to_s => Time.now.to_i.to_s - } - end - - it 'updates users.last_activity_on' do - subject.perform(data) - - aggregate_failures do - expect(user_active_2_days_ago.reload.last_activity_on).to eq(2.days.ago.to_date) - expect(user_active_yesterday_1.reload.last_activity_on).to eq(1.day.ago.to_date) - expect(user_active_yesterday_2.reload.last_activity_on).to eq(1.day.ago.to_date) - expect(user_active_today.reload.reload.last_activity_on).to eq(Date.today) - end - end - - it 'deletes the pairs from SharedState' do - data.each { |id, time| Gitlab::UserActivities.record(id, time) } - - subject.perform(data) - - expect(Gitlab::UserActivities.new.to_a).to be_empty - end -end -- cgit v1.2.1