Commit 92bacb99 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'remove-reset-project-activity-lease' into 'master'

Remove lease from Event#reset_project_activity

## What does this MR do?

This removes the exclusive lease used by `Event#reset_project_activity` in favour of conditional UPDATE queries. See dbcc623a901cb3fb725976217416bafad73dbf69 for more information.

## Why was this MR needed?

Obtaining the lease can, for whatever reason, be _really_ slow. See https://gitlab.com/gitlab-org/gitlab-ce/issues/22473 for more information.

https://gitlab.com/gitlab-org/gitlab-ce/issues/22473

See merge request !6678
parents 6948e1c8 c9bcfc63
...@@ -36,6 +36,7 @@ v 8.13.0 (unreleased) ...@@ -36,6 +36,7 @@ v 8.13.0 (unreleased)
- Add missing values to linter !6276 (Katarzyna Kobierska Ula Budziszewska) - Add missing values to linter !6276 (Katarzyna Kobierska Ula Budziszewska)
- Fix Long commit messages overflow viewport in file tree - Fix Long commit messages overflow viewport in file tree
- Revert avoid touching file system on Build#artifacts? - 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 - Add broadcast messages and alerts below sub-nav
- Better empty state for Groups view - Better empty state for Groups view
- Update ruby-prof to 0.16.2. !6026 (Elan Ruusamäe) - Update ruby-prof to 0.16.2. !6026 (Elan Ruusamäe)
......
...@@ -328,13 +328,15 @@ class Event < ActiveRecord::Base ...@@ -328,13 +328,15 @@ class Event < ActiveRecord::Base
def reset_project_activity def reset_project_activity
return unless project return unless project
# Don't even bother obtaining a lock if the last update happened less than # Don't bother updating if we know the project was updated recently.
# 60 minutes ago.
return if recent_update? return if recent_update?
return unless try_obtain_lease # At this point it's possible for multiple threads/processes to try to
# update the project. Only one query should actually perform the update,
project.update_column(:last_activity_at, created_at) # 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 end
private private
...@@ -342,11 +344,4 @@ class Event < ActiveRecord::Base ...@@ -342,11 +344,4 @@ class Event < ActiveRecord::Base
def recent_update? def recent_update?
project.last_activity_at > RESET_PROJECT_ACTIVITY_INTERVAL.ago project.last_activity_at > RESET_PROJECT_ACTIVITY_INTERVAL.ago
end 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 end
...@@ -173,13 +173,11 @@ describe Event, models: true do ...@@ -173,13 +173,11 @@ describe Event, models: true do
it 'updates the project' do it 'updates the project' do
project.update(last_activity_at: 1.year.ago) project.update(last_activity_at: 1.year.ago)
expect_any_instance_of(Gitlab::ExclusiveLease). create_event(project, project.owner)
to receive(:try_obtain).and_return(true)
expect(project).to receive(:update_column). project.reload
with(:last_activity_at, a_kind_of(Time))
create_event(project, project.owner) project.last_activity_at <= 1.minute.ago
end end
end end
end end
......
...@@ -308,8 +308,7 @@ describe Project, models: true do ...@@ -308,8 +308,7 @@ describe Project, models: true do
end end
describe 'last_activity methods' do describe 'last_activity methods' do
let(:timestamp) { Time.now - 2.hours } let(:project) { create(:project, last_activity_at: 2.hours.ago) }
let(:project) { create(:project, created_at: timestamp, updated_at: timestamp) }
describe 'last_activity' do describe 'last_activity' do
it 'alias last_activity to last_event' do it 'alias last_activity to last_event' do
...@@ -321,7 +320,6 @@ describe Project, models: true do ...@@ -321,7 +320,6 @@ describe Project, models: true do
describe 'last_activity_date' do describe 'last_activity_date' do
it 'returns the creation date of the project\'s last event if present' 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) 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) expect(project.last_activity_at.to_i).to eq(new_event.created_at.to_i)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment