Skip to content
Snippets Groups Projects
Commit 83bff1e9 authored by Albert's avatar Albert :red_circle:
Browse files

Refactor to move locks into CounterAttribute

parent ad00e3a6
No related branches found
No related tags found
No related merge requests found
This commit is part of merge request !97912. Comments created here will be created in the context of that merge request.
......@@ -47,6 +47,7 @@ module CounterAttribute
WORKER_DELAY = 10.minutes
WORKER_LOCK_TTL = 10.minutes
DATABASE_LOCK_RETRY_INTERVAL = 0.5
class_methods do
def counter_attribute(attribute)
......@@ -130,8 +131,14 @@ def increment_counter(attribute, increment)
end
end
def update_counters_with_lock(increments)
with_lock_for_update(log_fields: increments.merge({ caller: __method__ })) do
self.class.update_counters(id, increments)
end
end
def reset_counter!(attribute)
try_obtain_lock(log_fields: { caller: __method__ }) do
with_lock_for_update(log_fields: { caller: __method__ }) do
update!(attribute => 0)
clear_counter!(attribute)
end
......@@ -169,6 +176,19 @@ def counter_attribute_enabled?(attribute)
self.class.counter_attribute_enabled?(attribute)
end
def with_lock_for_update(log_fields: {})
Retriable.retriable(
tries: 3,
base_interval: DATABASE_LOCK_RETRY_INTERVAL,
on: ActiveRecord::LockWaitTimeout,
on_retry: retry_logger(log_fields)
) do
with_lock('FOR UPDATE NOWAIT') do
yield
end
end
end
private
def steal_increments(increment_key, flushed_key)
......@@ -230,4 +250,16 @@ def log_clear_counter(attribute)
Gitlab::AppLogger.info(payload)
end
def retry_logger(log_fields = {})
return proc {} unless Feature.enabled?(:warn_concurrent_project_statistics_update, type: :ops)
proc do
Gitlab::AppLogger.warn(
message: 'Concurrent update to project statistics detected',
project_statistics_id: id,
**log_fields
)
end
end
end
......@@ -28,8 +28,6 @@ class ProjectStatistics < ApplicationRecord
}.freeze
NAMESPACE_RELATABLE_COLUMNS = [:repository_size, :wiki_size, :lfs_objects_size, :uploads_size, :container_registry_size].freeze
LOCK_RETRY_INTERVAL = 0.5
scope :for_project_ids, ->(project_ids) { where(project_id: project_ids) }
scope :for_namespaces, -> (namespaces) { where(namespace: namespaces) }
......@@ -41,7 +39,7 @@ def total_repository_size
def refresh!(only: [])
return if Gitlab::Database.read_only?
try_obtain_lock(log_fields: { caller: __method__ }) do
with_lock_for_update(log_fields: { caller: __method__ }) do
COLUMNS_TO_REFRESH.each do |column, generator|
if only.empty? || only.include?(column)
public_send("update_#{column}") # rubocop:disable GitlabSecurity/PublicSend
......@@ -114,7 +112,7 @@ def update_storage_size
end
def refresh_storage_size!
try_obtain_lock(log_fields: { caller: __method__ }) do
with_lock_for_update(log_fields: { caller: __method__ }) do
update_storage_size
save!
end
......@@ -161,25 +159,6 @@ def increment_columns!(key, amount)
update_counters_with_lock(increments)
end
def update_counters_with_lock(increments)
try_obtain_lock(log_fields: increments.merge({ caller: __method__ })) do
self.class.update_counters(id, increments)
end
end
def try_obtain_lock(log_fields: {})
Retriable.retriable(
tries: 3,
base_interval: LOCK_RETRY_INTERVAL,
on: ActiveRecord::LockWaitTimeout,
on_retry: retry_logger(log_fields)
) do
with_lock('FOR UPDATE NOWAIT') do
yield
end
end
end
private
def schedule_namespace_aggregation_worker
......@@ -187,18 +166,6 @@ def schedule_namespace_aggregation_worker
Namespaces::ScheduleAggregationWorker.perform_async(project.namespace_id)
end
end
def retry_logger(log_fields = {})
return proc {} unless Feature.enabled?(:warn_concurrent_project_statistics_update, type: :ops)
proc do
Gitlab::AppLogger.warn(
message: 'Concurrent update to project statistics detected',
project_statistics_id: id,
**log_fields
)
end
end
end
ProjectStatistics.prepend_mod_with('ProjectStatistics')
......@@ -10,19 +10,6 @@
stub_const('ProjectStatistics::LOCK_RETRY_INTERVAL', 0.01)
end
shared_examples 'obtaining lock for update' do
context 'when it is unable to obtain lock' do
it 'retries 3 times and log a warning at each attempt' do
allow(statistics).to receive(:with_lock).and_raise(ActiveRecord::LockWaitTimeout)
expect(statistics).to receive(:with_lock).exactly(3).times
expect(Gitlab::AppLogger).to receive(:warn).exactly(3).times
expect { subject }.to raise_error(ActiveRecord::LockWaitTimeout)
end
end
end
describe 'associations' do
it { is_expected.to belong_to(:project) }
it { is_expected.to belong_to(:namespace) }
......@@ -261,7 +248,9 @@
end
end
it_behaves_like 'obtaining lock for update'
it_behaves_like 'obtaining database lock for update' do
let(:model) { statistics }
end
end
describe '#update_commit_count' do
......@@ -448,56 +437,8 @@
expect { subject }.to change { statistics.storage_size }.from(0).to(28)
end
it_behaves_like 'obtaining lock for update'
end
describe '#update_counters_with_lock' do
let(:increments) { { build_artifacts_size: 1, packages_size: 2 } }
subject { statistics.update_counters_with_lock(increments) }
it 'updates counters of the record' do
expect { subject }
.to change { statistics.reload.build_artifacts_size }.by(1)
.and change { statistics.reload.packages_size }.by(2)
end
it_behaves_like 'obtaining lock for update'
end
describe '#try_obtain_lock' do
context 'when it is unable to obtain lock' do
it 'retries 3 times and log a warning at each attempt' do
allow(statistics).to receive(:with_lock).and_raise(ActiveRecord::LockWaitTimeout)
expect(Gitlab::AppLogger).to receive(:warn).exactly(3).times
expect { statistics.try_obtain_lock }.to raise_error(ActiveRecord::LockWaitTimeout)
end
context 'when feature flag warn_concurrent_project_statistics_update is off' do
before do
stub_feature_flags(warn_concurrent_project_statistics_update: false)
end
it 'does not log a warning' do
allow(statistics).to receive(:with_lock).and_raise(ActiveRecord::LockWaitTimeout)
expect(Gitlab::AppLogger).not_to receive(:warn)
expect { statistics.try_obtain_lock }.to raise_error(ActiveRecord::LockWaitTimeout)
end
end
end
context 'when execution raises error' do
it 'does not retry to obtain lock and reraises error' do
allow(statistics).to receive(:with_lock).and_call_original
expect(statistics).to receive(:with_lock).once
expect { statistics.try_obtain_lock { raise StandardError, 'Something went wrong' } }
.to raise_error(StandardError, 'Something went wrong')
end
it_behaves_like 'obtaining database lock for update' do
let(:model) { statistics }
end
end
......
......@@ -190,7 +190,7 @@
let(:attribute) { counter_attributes.first }
before do
model.update(attribute => 123)
model.update!(attribute => 123)
model.increment_counter(attribute, 10)
end
......@@ -231,4 +231,67 @@
end
end
end
describe '#update_counters_with_lock' do
let(:increments) { { build_artifacts_size: 1, packages_size: 2 } }
subject { model.update_counters_with_lock(increments) }
it 'updates counters of the record' do
expect { subject }
.to change { model.reload.build_artifacts_size }.by(1)
.and change { model.reload.packages_size }.by(2)
end
it_behaves_like 'obtaining database lock for update'
end
describe '#with_lock_for_update' do
context 'when it is unable to obtain lock' do
it 'retries 3 times and log a warning at each attempt' do
allow(model).to receive(:with_lock).and_raise(ActiveRecord::LockWaitTimeout)
expect(Gitlab::AppLogger).to receive(:warn).exactly(3).times
expect { model.with_lock_for_update }.to raise_error(ActiveRecord::LockWaitTimeout)
end
context 'when feature flag warn_concurrent_project_statistics_update is off' do
before do
stub_feature_flags(warn_concurrent_project_statistics_update: false)
end
it 'does not log a warning' do
allow(model).to receive(:with_lock).and_raise(ActiveRecord::LockWaitTimeout)
expect(Gitlab::AppLogger).not_to receive(:warn)
expect { model.with_lock_for_update }.to raise_error(ActiveRecord::LockWaitTimeout)
end
end
end
context 'when execution raises error' do
it 'does not retry to obtain lock and reraises error' do
allow(model).to receive(:with_lock).and_call_original
expect(model).to receive(:with_lock).once
expect { model.with_lock_for_update { raise StandardError, 'Something went wrong' } }
.to raise_error(StandardError, 'Something went wrong')
end
end
end
end
RSpec.shared_examples 'obtaining database lock for update' do
context 'when it is unable to obtain lock' do
it 'retries 3 times and log a warning at each attempt' do
allow(model).to receive(:with_lock).and_raise(ActiveRecord::LockWaitTimeout)
expect(model).to receive(:with_lock).exactly(3).times
expect(Gitlab::AppLogger).to receive(:warn).exactly(3).times
expect { subject }.to raise_error(ActiveRecord::LockWaitTimeout)
end
end
end
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment