Commit 102b3232 authored by Sean McGivern's avatar Sean McGivern

Merge branch 'throttle-touching-of-objects' into 'master'

Throttle the number of UPDATEs triggered by touch

Closes #34763

See merge request gitlab-org/gitlab-ce!15682
parents d87d1e79 856447cc
No related merge requests found
# ThrottledTouch can be used to throttle the number of updates triggered by
# calling "touch" on an ActiveRecord model.
module ThrottledTouch
# The amount of time to wait before "touch" can update a record again.
TOUCH_INTERVAL = 1.minute
def touch(*args)
super if (Time.zone.now - updated_at) > TOUCH_INTERVAL
end
end
......@@ -9,6 +9,7 @@ class Issue < ActiveRecord::Base
include FasterCacheKeys
include RelativePositioning
include TimeTrackable
include ThrottledTouch
DueDateStruct = Struct.new(:title, :name).freeze
NoDueDate = DueDateStruct.new('No Due Date', '0').freeze
......
......@@ -7,6 +7,7 @@ class MergeRequest < ActiveRecord::Base
include TimeTrackable
include ManualInverseAssociation
include EachBatch
include ThrottledTouch
ignore_column :locked_at,
:ref_fetched
......
......@@ -15,6 +15,7 @@ class Note < ActiveRecord::Base
include IgnorableColumn
include Editable
include Gitlab::SQL::Pattern
include ThrottledTouch
module SpecialRole
FIRST_TIME_CONTRIBUTOR = :first_time_contributor
......@@ -55,7 +56,7 @@ class Note < ActiveRecord::Base
participant :author
belongs_to :project
belongs_to :noteable, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :noteable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :author, class_name: "User"
belongs_to :updated_by, class_name: "User"
belongs_to :last_edited_by, class_name: 'User'
......@@ -118,6 +119,7 @@ class Note < ActiveRecord::Base
before_validation :set_discussion_id, on: :create
after_save :keep_around_commit, if: :for_project_noteable?
after_save :expire_etag_cache
after_save :touch_noteable
after_destroy :expire_etag_cache
class << self
......@@ -367,6 +369,38 @@ class Note < ActiveRecord::Base
Gitlab::EtagCaching::Store.new.touch(key)
end
def touch(*args)
# We're not using an explicit transaction here because this would in all
# cases result in all future queries going to the primary, even if no writes
# are performed.
#
# We touch the noteable first so its SELECT query can run before our writes,
# ensuring it runs on a secondary (if no prior write took place).
touch_noteable
super
end
# By default Rails will issue an "SELECT *" for the relation, which is
# overkill for just updating the timestamps. To work around this we manually
# touch the data so we can SELECT only the columns we need.
def touch_noteable
# Commits are not stored in the DB so we can't touch them.
return if for_commit?
assoc = association(:noteable)
noteable_object =
if assoc.loaded?
noteable
else
# If the object is not loaded (e.g. when notes are loaded async) we
# _only_ want the data we actually need.
assoc.scope.select(:id, :updated_at).take
end
noteable_object&.touch
end
private
def keep_around_commit
......
---
title: Throttle the number of UPDATEs triggered by touch
merge_request:
author:
type: performance
......@@ -171,7 +171,7 @@ describe Issuable do
it "returns false when record has been updated" do
allow(issue).to receive(:today?).and_return(true)
issue.touch
issue.update_attribute(:updated_at, 1.hour.ago)
expect(issue.new?).to be_falsey
end
end
......
......@@ -765,4 +765,8 @@ describe Issue do
expect(described_class.public_only).to eq([public_issue])
end
end
it_behaves_like 'throttled touch' do
subject { create(:issue, updated_at: 1.hour.ago) }
end
end
......@@ -1851,4 +1851,8 @@ describe MergeRequest do
.to change { project.open_merge_requests_count }.from(1).to(0)
end
end
it_behaves_like 'throttled touch' do
subject { create(:merge_request, updated_at: 1.hour.ago) }
end
end
......@@ -5,7 +5,7 @@ describe Note do
describe 'associations' do
it { is_expected.to belong_to(:project) }
it { is_expected.to belong_to(:noteable).touch(true) }
it { is_expected.to belong_to(:noteable).touch(false) }
it { is_expected.to belong_to(:author).class_name('User') }
it { is_expected.to have_many(:todos).dependent(:destroy) }
......
shared_examples_for 'throttled touch' do
describe '#touch' do
it 'updates the updated_at timestamp' do
Timecop.freeze do
subject.touch
expect(subject.updated_at).to eq(Time.zone.now)
end
end
it 'updates the object at most once per minute' do
first_updated_at = Time.zone.now - (ThrottledTouch::TOUCH_INTERVAL * 2)
second_updated_at = Time.zone.now - (ThrottledTouch::TOUCH_INTERVAL * 1.5)
Timecop.freeze(first_updated_at) { subject.touch }
Timecop.freeze(second_updated_at) { subject.touch }
expect(subject.updated_at).to eq(first_updated_at)
end
end
end
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