From 0adedbb4822a8daaa215b33c88ace136c31d042d Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Mon, 1 Apr 2019 15:11:08 +0900 Subject: Fix the bug that the project statistics is not updated --- app/workers/all_queues.yml | 1 + app/workers/project_cache_worker.rb | 14 ++++--- app/workers/update_project_statistics_worker.rb | 22 ++++++++++ changelogs/unreleased/delay-update-statictics.yml | 5 +++ config/sidekiq_queues.yml | 3 +- spec/workers/project_cache_worker_spec.rb | 36 +++++++++------- .../update_project_statistics_worker_spec.rb | 48 ++++++++++++++++++++++ 7 files changed, 107 insertions(+), 22 deletions(-) create mode 100644 app/workers/update_project_statistics_worker.rb create mode 100644 changelogs/unreleased/delay-update-statictics.yml create mode 100644 spec/workers/update_project_statistics_worker_spec.rb diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 3e8c2a1209a..f9b2e698fc9 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -143,6 +143,7 @@ - repository_remove_remote - system_hook_push - update_merge_requests +- update_project_statistics - upload_checksum - web_hook - repository_update_remote_mirror diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index b31099bc670..73eb461768c 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -28,18 +28,20 @@ class ProjectCacheWorker def update_statistics(project, statistics = []) return if Gitlab::Database.read_only? - return unless try_obtain_lease_for(project.id, :update_statistics) + return unless try_obtain_lease_for(project.id, statistics) - Rails.logger.info("Updating statistics for project #{project.id}") - - project.statistics.refresh!(only: statistics) + UpdateProjectStatisticsWorker.perform_in(LEASE_TIMEOUT, project.id, statistics) end private - def try_obtain_lease_for(project_id, section) + def try_obtain_lease_for(project_id, statistics) Gitlab::ExclusiveLease - .new("project_cache_worker:#{project_id}:#{section}", timeout: LEASE_TIMEOUT) + .new(project_cache_worker_key(project_id, statistics), timeout: LEASE_TIMEOUT) .try_obtain end + + def project_cache_worker_key(project_id, statistics) + (["project_cache_worker", project_id] + statistics.sort).join(":") + end end diff --git a/app/workers/update_project_statistics_worker.rb b/app/workers/update_project_statistics_worker.rb new file mode 100644 index 00000000000..46673c4b07e --- /dev/null +++ b/app/workers/update_project_statistics_worker.rb @@ -0,0 +1,22 @@ + +# frozen_string_literal: true + +# Worker for updating project statistics. +class UpdateProjectStatisticsWorker + include ApplicationWorker + + # project_id - The ID of the project for which to flush the cache. + # statistics - An Array containing columns from ProjectStatistics to + # refresh, if empty all columns will be refreshed + # rubocop: disable CodeReuse/ActiveRecord + def perform(project_id, statistics = []) + project = Project.find_by(id: project_id) + + return unless project && project.repository.exists? + + Rails.logger.info("Updating statistics for project #{project.id}") + + project.statistics.refresh!(only: statistics.map(&:to_sym)) + end + # rubocop: enable CodeReuse/ActiveRecord +end diff --git a/changelogs/unreleased/delay-update-statictics.yml b/changelogs/unreleased/delay-update-statictics.yml new file mode 100644 index 00000000000..d0201fb6db8 --- /dev/null +++ b/changelogs/unreleased/delay-update-statictics.yml @@ -0,0 +1,5 @@ +--- +title: Fix the bug that the project statistics is not updated +merge_request: 26854 +author: Hiroyuki Sato +type: fixed diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 2dc0da00919..8bc2426ec4c 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -89,4 +89,5 @@ - [project_daily_statistics, 1] - [import_issues_csv, 2] - [chat_notification, 2] - - [migrate_external_diffs, 1] + - [migrate_external_diffs, 1] + - [update_project_statistics, 1] diff --git a/spec/workers/project_cache_worker_spec.rb b/spec/workers/project_cache_worker_spec.rb index a7353227043..724947228b7 100644 --- a/spec/workers/project_cache_worker_spec.rb +++ b/spec/workers/project_cache_worker_spec.rb @@ -7,9 +7,9 @@ describe ProjectCacheWorker do let(:worker) { described_class.new } let(:project) { create(:project, :repository) } - let(:statistics) { project.statistics } - let(:lease_key) { "project_cache_worker:#{project.id}:update_statistics" } + let(:lease_key) { (["project_cache_worker", project.id] + statistics.sort).join(":") } let(:lease_timeout) { ProjectCacheWorker::LEASE_TIMEOUT } + let(:statistics) { [] } describe '#perform' do before do @@ -35,14 +35,6 @@ describe ProjectCacheWorker do end context 'with an existing project' do - it 'updates the project statistics' do - expect(worker).to receive(:update_statistics) - .with(kind_of(Project), %i(repository_size)) - .and_call_original - - worker.perform(project.id, [], %w(repository_size)) - end - it 'refreshes the method caches' do expect_any_instance_of(Repository).to receive(:refresh_method_caches) .with(%i(readme)) @@ -51,6 +43,18 @@ describe ProjectCacheWorker do worker.perform(project.id, %w(readme)) end + context 'with statistics' do + let(:statistics) { %w(repository_size) } + + it 'updates the project statistics' do + expect(worker).to receive(:update_statistics) + .with(kind_of(Project), %i(repository_size)) + .and_call_original + + worker.perform(project.id, [], statistics) + end + end + context 'with plain readme' do it 'refreshes the method caches' do allow(MarkupHelper).to receive(:gitlab_markdown?).and_return(false) @@ -66,13 +70,15 @@ describe ProjectCacheWorker do end describe '#update_statistics' do + let(:statistics) { %w(repository_size) } + context 'when a lease could not be obtained' do it 'does not update the repository size' do stub_exclusive_lease_taken(lease_key, timeout: lease_timeout) - expect(statistics).not_to receive(:refresh!) + expect(UpdateProjectStatisticsWorker).not_to receive(:perform_in) - worker.update_statistics(project) + worker.update_statistics(project, statistics.map(&:to_sym)) end end @@ -80,11 +86,11 @@ describe ProjectCacheWorker do it 'updates the project statistics' do stub_exclusive_lease(lease_key, timeout: lease_timeout) - expect(statistics).to receive(:refresh!) - .with(only: %i(repository_size)) + expect(UpdateProjectStatisticsWorker).to receive(:perform_in) + .with(lease_timeout, project.id, statistics.map(&:to_sym)) .and_call_original - worker.update_statistics(project, %i(repository_size)) + worker.update_statistics(project, statistics.map(&:to_sym)) end end end diff --git a/spec/workers/update_project_statistics_worker_spec.rb b/spec/workers/update_project_statistics_worker_spec.rb new file mode 100644 index 00000000000..3411e10da7e --- /dev/null +++ b/spec/workers/update_project_statistics_worker_spec.rb @@ -0,0 +1,48 @@ +require 'spec_helper' + +describe UpdateProjectStatisticsWorker do + let(:worker) { described_class.new } + let(:project) { create(:project, :repository) } + + describe '#perform' do + context 'with a non-existing project' do + it 'does nothing' do + expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) + + worker.perform(-1) + end + end + + context 'with an existing project without a repository' do + it 'does nothing' do + allow_any_instance_of(Repository).to receive(:exists?).and_return(false) + + expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) + + worker.perform(project.id) + end + end + + context 'with an existing project' do + it 'refreshes the project statistics' do + expect_any_instance_of(ProjectStatistics).to receive(:refresh!) + .with(only: []) + .and_call_original + + worker.perform(project.id) + end + + context 'with a specific statistics target' do + it 'refreshes the project repository size' do + statistics_target = %w(repository_size) + + expect_any_instance_of(ProjectStatistics).to receive(:refresh!) + .with(only: statistics_target.map(&:to_sym)) + .and_call_original + + worker.perform(project.id, statistics_target) + end + end + end + end +end -- cgit v1.2.1 From e6780501cbfd0cae31fadd15b2964204171091cc Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Fri, 5 Apr 2019 00:17:52 +0900 Subject: Refactor project_cache_worker_key --- app/workers/project_cache_worker.rb | 2 +- spec/workers/project_cache_worker_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index 73eb461768c..2b25b96a6f8 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -42,6 +42,6 @@ class ProjectCacheWorker end def project_cache_worker_key(project_id, statistics) - (["project_cache_worker", project_id] + statistics.sort).join(":") + ["project_cache_worker", project_id, *statistics.sort].join(":") end end diff --git a/spec/workers/project_cache_worker_spec.rb b/spec/workers/project_cache_worker_spec.rb index 724947228b7..7053bfc2bfd 100644 --- a/spec/workers/project_cache_worker_spec.rb +++ b/spec/workers/project_cache_worker_spec.rb @@ -7,7 +7,7 @@ describe ProjectCacheWorker do let(:worker) { described_class.new } let(:project) { create(:project, :repository) } - let(:lease_key) { (["project_cache_worker", project.id] + statistics.sort).join(":") } + let(:lease_key) { ["project_cache_worker", project.id, *statistics.sort].join(":") } let(:lease_timeout) { ProjectCacheWorker::LEASE_TIMEOUT } let(:statistics) { [] } -- cgit v1.2.1 From 074a1797fe581c8eb5cc65bd56af584d5c500688 Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Fri, 5 Apr 2019 00:18:17 +0900 Subject: Update the project statistics immediatelly --- app/workers/project_cache_worker.rb | 7 +++++++ spec/workers/project_cache_worker_spec.rb | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index 2b25b96a6f8..5f4972d8f93 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -26,10 +26,17 @@ class ProjectCacheWorker end # rubocop: enable CodeReuse/ActiveRecord + # NOTE: triggering both an immediate update and one in 15 minutes if we + # successfully obtain the lease. That way, we only need to wait for the + # statistics to become accurate if they were already updated once in the + # last 15 minutes. def update_statistics(project, statistics = []) return if Gitlab::Database.read_only? return unless try_obtain_lease_for(project.id, statistics) + Rails.logger.info("Updating statistics for project #{project.id}") + + project.statistics.refresh!(only: statistics) UpdateProjectStatisticsWorker.perform_in(LEASE_TIMEOUT, project.id, statistics) end diff --git a/spec/workers/project_cache_worker_spec.rb b/spec/workers/project_cache_worker_spec.rb index 7053bfc2bfd..d2445f420f8 100644 --- a/spec/workers/project_cache_worker_spec.rb +++ b/spec/workers/project_cache_worker_spec.rb @@ -86,6 +86,10 @@ describe ProjectCacheWorker do it 'updates the project statistics' do stub_exclusive_lease(lease_key, timeout: lease_timeout) + expect(project.statistics).to receive(:refresh!) + .with(only: statistics.map(&:to_sym)) + .and_call_original + expect(UpdateProjectStatisticsWorker).to receive(:perform_in) .with(lease_timeout, project.id, statistics.map(&:to_sym)) .and_call_original -- cgit v1.2.1 From 770f721962cd30e930ab7b6e06e9386da6325c3c Mon Sep 17 00:00:00 2001 From: Hiroyuki Sato Date: Fri, 5 Apr 2019 22:07:09 +0900 Subject: Refactor: extract duplicate steps to a service class --- app/services/projects/update_statistics_service.rb | 19 ++++++++++ app/workers/project_cache_worker.rb | 5 +-- app/workers/update_project_statistics_worker.rb | 6 +-- .../projects/update_statistics_service_spec.rb | 40 ++++++++++++++++++++ spec/workers/project_cache_worker_spec.rb | 19 ++++++---- .../update_project_statistics_worker_spec.rb | 43 +++------------------- 6 files changed, 79 insertions(+), 53 deletions(-) create mode 100644 app/services/projects/update_statistics_service.rb create mode 100644 spec/services/projects/update_statistics_service_spec.rb diff --git a/app/services/projects/update_statistics_service.rb b/app/services/projects/update_statistics_service.rb new file mode 100644 index 00000000000..f32a779fab0 --- /dev/null +++ b/app/services/projects/update_statistics_service.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +module Projects + class UpdateStatisticsService < BaseService + def execute + return unless project && project.repository.exists? + + Rails.logger.info("Updating statistics for project #{project.id}") + + project.statistics.refresh!(only: statistics.map(&:to_sym)) + end + + private + + def statistics + params[:statistics] + end + end +end diff --git a/app/workers/project_cache_worker.rb b/app/workers/project_cache_worker.rb index 5f4972d8f93..b2e0701008a 100644 --- a/app/workers/project_cache_worker.rb +++ b/app/workers/project_cache_worker.rb @@ -18,7 +18,7 @@ class ProjectCacheWorker return unless project && project.repository.exists? - update_statistics(project, statistics.map(&:to_sym)) + update_statistics(project, statistics) project.repository.refresh_method_caches(files.map(&:to_sym)) @@ -34,9 +34,8 @@ class ProjectCacheWorker return if Gitlab::Database.read_only? return unless try_obtain_lease_for(project.id, statistics) - Rails.logger.info("Updating statistics for project #{project.id}") + Projects::UpdateStatisticsService.new(project, nil, statistics: statistics).execute - project.statistics.refresh!(only: statistics) UpdateProjectStatisticsWorker.perform_in(LEASE_TIMEOUT, project.id, statistics) end diff --git a/app/workers/update_project_statistics_worker.rb b/app/workers/update_project_statistics_worker.rb index 46673c4b07e..9a29cc12707 100644 --- a/app/workers/update_project_statistics_worker.rb +++ b/app/workers/update_project_statistics_worker.rb @@ -12,11 +12,7 @@ class UpdateProjectStatisticsWorker def perform(project_id, statistics = []) project = Project.find_by(id: project_id) - return unless project && project.repository.exists? - - Rails.logger.info("Updating statistics for project #{project.id}") - - project.statistics.refresh!(only: statistics.map(&:to_sym)) + Projects::UpdateStatisticsService.new(project, nil, statistics: statistics).execute end # rubocop: enable CodeReuse/ActiveRecord end diff --git a/spec/services/projects/update_statistics_service_spec.rb b/spec/services/projects/update_statistics_service_spec.rb new file mode 100644 index 00000000000..7e351c9ce54 --- /dev/null +++ b/spec/services/projects/update_statistics_service_spec.rb @@ -0,0 +1,40 @@ +require 'spec_helper' + +describe Projects::UpdateStatisticsService do + let(:service) { described_class.new(project, nil, statistics: statistics)} + let(:statistics) { %w(repository_size) } + + describe '#execute' do + context 'with a non-existing project' do + let(:project) { nil } + + it 'does nothing' do + expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) + + service.execute + end + end + + context 'with an existing project without a repository' do + let(:project) { create(:project) } + + it 'does nothing' do + expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) + + service.execute + end + end + + context 'with an existing project with a repository' do + let(:project) { create(:project, :repository) } + + it 'refreshes the project statistics' do + expect_any_instance_of(ProjectStatistics).to receive(:refresh!) + .with(only: statistics.map(&:to_sym)) + .and_call_original + + service.execute + end + end + end +end diff --git a/spec/workers/project_cache_worker_spec.rb b/spec/workers/project_cache_worker_spec.rb index d2445f420f8..3c40269adc7 100644 --- a/spec/workers/project_cache_worker_spec.rb +++ b/spec/workers/project_cache_worker_spec.rb @@ -48,7 +48,7 @@ describe ProjectCacheWorker do it 'updates the project statistics' do expect(worker).to receive(:update_statistics) - .with(kind_of(Project), %i(repository_size)) + .with(kind_of(Project), statistics) .and_call_original worker.perform(project.id, [], statistics) @@ -73,28 +73,31 @@ describe ProjectCacheWorker do let(:statistics) { %w(repository_size) } context 'when a lease could not be obtained' do - it 'does not update the repository size' do + it 'does not update the project statistics' do stub_exclusive_lease_taken(lease_key, timeout: lease_timeout) + expect(Projects::UpdateStatisticsService).not_to receive(:new) + expect(UpdateProjectStatisticsWorker).not_to receive(:perform_in) - worker.update_statistics(project, statistics.map(&:to_sym)) + worker.update_statistics(project, statistics) end end context 'when a lease could be obtained' do - it 'updates the project statistics' do + it 'updates the project statistics twice' do stub_exclusive_lease(lease_key, timeout: lease_timeout) - expect(project.statistics).to receive(:refresh!) - .with(only: statistics.map(&:to_sym)) + expect(Projects::UpdateStatisticsService).to receive(:new) + .with(project, nil, statistics: statistics) .and_call_original + .twice expect(UpdateProjectStatisticsWorker).to receive(:perform_in) - .with(lease_timeout, project.id, statistics.map(&:to_sym)) + .with(lease_timeout, project.id, statistics) .and_call_original - worker.update_statistics(project, statistics.map(&:to_sym)) + worker.update_statistics(project, statistics) end end end diff --git a/spec/workers/update_project_statistics_worker_spec.rb b/spec/workers/update_project_statistics_worker_spec.rb index 3411e10da7e..a268fd2e4ba 100644 --- a/spec/workers/update_project_statistics_worker_spec.rb +++ b/spec/workers/update_project_statistics_worker_spec.rb @@ -3,46 +3,15 @@ require 'spec_helper' describe UpdateProjectStatisticsWorker do let(:worker) { described_class.new } let(:project) { create(:project, :repository) } + let(:statistics) { %w(repository_size) } describe '#perform' do - context 'with a non-existing project' do - it 'does nothing' do - expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) + it 'updates the project statistics' do + expect(Projects::UpdateStatisticsService).to receive(:new) + .with(project, nil, statistics: statistics) + .and_call_original - worker.perform(-1) - end - end - - context 'with an existing project without a repository' do - it 'does nothing' do - allow_any_instance_of(Repository).to receive(:exists?).and_return(false) - - expect_any_instance_of(ProjectStatistics).not_to receive(:refresh!) - - worker.perform(project.id) - end - end - - context 'with an existing project' do - it 'refreshes the project statistics' do - expect_any_instance_of(ProjectStatistics).to receive(:refresh!) - .with(only: []) - .and_call_original - - worker.perform(project.id) - end - - context 'with a specific statistics target' do - it 'refreshes the project repository size' do - statistics_target = %w(repository_size) - - expect_any_instance_of(ProjectStatistics).to receive(:refresh!) - .with(only: statistics_target.map(&:to_sym)) - .and_call_original - - worker.perform(project.id, statistics_target) - end - end + worker.perform(project.id, statistics) end end end -- cgit v1.2.1