Merge branch 'delay-update-statictics' into 'master'

Fix the bug that the project statistics is not updated See merge request gitlab-org/gitlab-ce!26854

Merge branch 'delay-update-statictics' into 'master'
Fix the bug that the project statistics is not updated See merge request gitlab-org/gitlab-ce!26854
ca03848d · Douwe Maan · 87f665e8 · 770f7219 · ca03848d · ca03848d
--- a/app/services/projects/update_statistics_service.rb
+++ b/app/services/projects/update_statistics_service.rb
+# 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
--- 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

--- a/app/workers/project_cache_worker.rb
+++ b/app/workers/project_cache_worker.rb
@@ -18,7 +18,7 @@ def perform(project_id, files = [], statistics = [])

    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))

@@ -26,20 +26,28 @@ def perform(project_id, files = [], statistics = [])
  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, :update_statistics)
+    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

  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
--- a/app/workers/update_project_statistics_worker.rb
+++ b/app/workers/update_project_statistics_worker.rb
+
+# 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)
+
+    Projects::UpdateStatisticsService.new(project, nil, statistics: statistics).execute
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+end
--- a/changelogs/unreleased/delay-update-statictics.yml
+++ b/changelogs/unreleased/delay-update-statictics.yml
+---
+title: Fix the bug that the project statistics is not updated
+merge_request: 26854
+author: Hiroyuki Sato
+type: fixed
--- 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]
--- a/spec/services/projects/update_statistics_service_spec.rb
+++ b/spec/services/projects/update_statistics_service_spec.rb
+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
--- a/spec/workers/project_cache_worker_spec.rb
+++ b/spec/workers/project_cache_worker_spec.rb
@@ -7,9 +7,9 @@

  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 @@
    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 @@
        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), statistics)
+            .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,25 +70,34 @@
  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
+      it 'does not update the project statistics' do
        stub_exclusive_lease_taken(lease_key, timeout: lease_timeout)

-        expect(statistics).not_to receive(:refresh!)
+        expect(Projects::UpdateStatisticsService).not_to receive(:new)

-        worker.update_statistics(project)
+        expect(UpdateProjectStatisticsWorker).not_to receive(:perform_in)
+
+        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(statistics).to receive(:refresh!)
-          .with(only: %i(repository_size))
+        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)
          .and_call_original

-        worker.update_statistics(project, %i(repository_size))
+        worker.update_statistics(project, statistics)
      end
    end
  end

--- a/spec/workers/update_project_statistics_worker_spec.rb
+++ b/spec/workers/update_project_statistics_worker_spec.rb
+require 'spec_helper'
+
+describe UpdateProjectStatisticsWorker do
+  let(:worker) { described_class.new }
+  let(:project) { create(:project, :repository) }
+  let(:statistics) { %w(repository_size) }
+
+  describe '#perform' do
+    it 'updates the project statistics' do
+      expect(Projects::UpdateStatisticsService).to receive(:new)
+        .with(project, nil, statistics: statistics)
+        .and_call_original
+
+      worker.perform(project.id, statistics)
+    end
+  end
+end