summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorYorick Peterse <yorickpeterse@gmail.com>2016-10-04 17:22:58 +0200
committerYorick Peterse <yorickpeterse@gmail.com>2016-10-04 19:41:37 +0200
commitc9bcfc631a79db97203d51d7c221a7549ba65adf (patch)
tree01a8be8de90c498e916bc9f6d8c2bc7478e89226
parent7887a3dafb6902cc39c831e307c48faf49f80b51 (diff)
downloadgitlab-ce-c9bcfc631a79db97203d51d7c221a7549ba65adf.tar.gz
Remove lease from Event#reset_project_activityremove-reset-project-activity-lease
Per GitLab.com's performance metrics this method could take up to 5 seconds of wall time to complete, while only taking 1-2 milliseconds of CPU time. Removing the Redis lease in favour of conditional updates allows us to work around this. A slight drawback is that this allows for multiple threads/processes to try and update the same row. However, only a single thread/process will ever win since the UPDATE query uses a WHERE condition to only update rows that were not updated in the last hour. Fixes gitlab-org/gitlab-ce#22473
-rw-r--r--CHANGELOG1
-rw-r--r--app/models/event.rb19
-rw-r--r--spec/models/event_spec.rb8
-rw-r--r--spec/models/project_spec.rb4
4 files changed, 12 insertions, 20 deletions
diff --git a/CHANGELOG b/CHANGELOG
index a47ec511452..3e66c4c8336 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -35,6 +35,7 @@ v 8.13.0 (unreleased)
- Add missing values to linter !6276 (Katarzyna Kobierska Ula Budziszewska)
- Fix Long commit messages overflow viewport in file tree
- Revert avoid touching file system on Build#artifacts?
+ - Stop using a Redis lease when updating the project activity timestamp whenever a new event is created
- Add broadcast messages and alerts below sub-nav
- Better empty state for Groups view
- Update ruby-prof to 0.16.2. !6026 (Elan Ruusamäe)
diff --git a/app/models/event.rb b/app/models/event.rb
index 55a76e26f3c..633019fe0af 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -328,13 +328,15 @@ class Event < ActiveRecord::Base
def reset_project_activity
return unless project
- # Don't even bother obtaining a lock if the last update happened less than
- # 60 minutes ago.
+ # Don't bother updating if we know the project was updated recently.
return if recent_update?
- return unless try_obtain_lease
-
- project.update_column(:last_activity_at, created_at)
+ # At this point it's possible for multiple threads/processes to try to
+ # update the project. Only one query should actually perform the update,
+ # hence we add the extra WHERE clause for last_activity_at.
+ Project.unscoped.where(id: project_id).
+ where('last_activity_at > ?', RESET_PROJECT_ACTIVITY_INTERVAL.ago).
+ update_all(last_activity_at: created_at)
end
private
@@ -342,11 +344,4 @@ class Event < ActiveRecord::Base
def recent_update?
project.last_activity_at > RESET_PROJECT_ACTIVITY_INTERVAL.ago
end
-
- def try_obtain_lease
- Gitlab::ExclusiveLease.
- new("project:update_last_activity_at:#{project.id}",
- timeout: RESET_PROJECT_ACTIVITY_INTERVAL.to_i).
- try_obtain
- end
end
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index 8600eb4d2c4..af5002487cc 100644
--- a/spec/models/event_spec.rb
+++ b/spec/models/event_spec.rb
@@ -173,13 +173,11 @@ describe Event, models: true do
it 'updates the project' do
project.update(last_activity_at: 1.year.ago)
- expect_any_instance_of(Gitlab::ExclusiveLease).
- to receive(:try_obtain).and_return(true)
+ create_event(project, project.owner)
- expect(project).to receive(:update_column).
- with(:last_activity_at, a_kind_of(Time))
+ project.reload
- create_event(project, project.owner)
+ project.last_activity_at <= 1.minute.ago
end
end
end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index ef854a25321..3ab5ac78bba 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -308,8 +308,7 @@ describe Project, models: true do
end
describe 'last_activity methods' do
- let(:timestamp) { Time.now - 2.hours }
- let(:project) { create(:project, created_at: timestamp, updated_at: timestamp) }
+ let(:project) { create(:project, last_activity_at: 2.hours.ago) }
describe 'last_activity' do
it 'alias last_activity to last_event' do
@@ -321,7 +320,6 @@ describe Project, models: true do
describe 'last_activity_date' do
it 'returns the creation date of the project\'s last event if present' do
- expect_any_instance_of(Event).to receive(:try_obtain_lease).and_return(true)
new_event = create(:event, project: project, created_at: Time.now)
expect(project.last_activity_at.to_i).to eq(new_event.created_at.to_i)