diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2017-09-20 14:21:15 +0200 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2017-09-20 14:24:14 +0200 |
commit | b3566a01049cdfbde2a9221319601d8949c12a5a (patch) | |
tree | 873d695caaaec68afabf8146af5f83f0983be04e /spec | |
parent | a09d032b2a64c7b6652dcd589de2d9bcba7d9613 (diff) | |
download | gitlab-ce-b3566a01049cdfbde2a9221319601d8949c12a5a.tar.gz |
Stop using Sidekiq for updating Key#last_used_atremove-use-key-worker
This makes things simpler as no scheduling is involved. Further we
remove the need for running a SELECT + UPDATE just to get the key and
update it, whereas we only need an UPDATE when setting last_used_at
directly in a request.
The added service class takes care of updating Key#last_used_at without
using Sidekiq. Further it makes sure we only try to obtain a Redis lease
if we're confident that we actually need to do so, instead of always
obtaining it. We also make sure to _only_ update last_used_at instead of
also updating updated_at.
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/36663
Diffstat (limited to 'spec')
-rw-r--r-- | spec/models/key_spec.rb | 31 | ||||
-rw-r--r-- | spec/services/keys/last_used_service_spec.rb | 68 | ||||
-rw-r--r-- | spec/workers/use_key_worker_spec.rb | 23 |
3 files changed, 77 insertions, 45 deletions
diff --git a/spec/models/key_spec.rb b/spec/models/key_spec.rb index dbc4aba8547..8eabc4ca72f 100644 --- a/spec/models/key_spec.rb +++ b/spec/models/key_spec.rb @@ -37,30 +37,17 @@ describe Key, :mailer do end describe "#update_last_used_at" do - let(:key) { create(:key) } - - context 'when key was not updated during the last day' do - before do - allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) - .and_return('000000') - end - - it 'enqueues a UseKeyWorker job' do - expect(UseKeyWorker).to receive(:perform_async).with(key.id) - key.update_last_used_at - end - end + it 'updates the last used timestamp' do + key = build(:key) + service = double(:service) + + expect(Keys::LastUsedService).to receive(:new) + .with(key) + .and_return(service) - context 'when key was updated during the last day' do - before do - allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain) - .and_return(false) - end + expect(service).to receive(:execute) - it 'does not enqueue a UseKeyWorker job' do - expect(UseKeyWorker).not_to receive(:perform_async) - key.update_last_used_at - end + key.update_last_used_at end end end diff --git a/spec/services/keys/last_used_service_spec.rb b/spec/services/keys/last_used_service_spec.rb new file mode 100644 index 00000000000..edaace293ae --- /dev/null +++ b/spec/services/keys/last_used_service_spec.rb @@ -0,0 +1,68 @@ +require 'spec_helper' + +describe Keys::LastUsedService do + describe '#execute', :clean_gitlab_redis_shared_state do + it 'updates the key when it has not been used recently' do + key = create(:key, last_used_at: 1.year.ago) + time = Time.zone.now + + Timecop.freeze(time) { described_class.new(key).execute } + + expect(key.last_used_at).to eq(time) + end + + it 'does not update the key when it has been used recently' do + time = 1.minute.ago + key = create(:key, last_used_at: time) + + described_class.new(key).execute + + expect(key.last_used_at).to eq(time) + end + + it 'does not update the updated_at field' do + # Since a lot of these updates could happen in parallel for different keys + # we want these updates to be as lightweight as possible, hence we want to + # make sure we _only_ update last_used_at and not always updated_at. + key = create(:key, last_used_at: 1.year.ago) + + recorder = ActiveRecord::QueryRecorder.new do + described_class.new(key).execute + end + + expect(recorder.count).to eq(1) + expect(recorder.log[0]).not_to include('updated_at') + end + end + + describe '#update?', :clean_gitlab_redis_shared_state do + it 'returns true when no last used timestamp is present' do + key = build(:key, last_used_at: nil) + service = described_class.new(key) + + expect(service.update?).to eq(true) + end + + it 'returns true when the key needs to be updated' do + key = build(:key, last_used_at: 1.year.ago) + service = described_class.new(key) + + expect(service.update?).to eq(true) + end + + it 'returns false when a lease has already been obtained' do + key = build(:key, last_used_at: 1.year.ago) + service = described_class.new(key) + + expect(service.update?).to eq(true) + expect(service.update?).to eq(false) + end + + it 'returns false when the key does not yet need to be updated' do + key = build(:key, last_used_at: 1.minute.ago) + service = described_class.new(key) + + expect(service.update?).to eq(false) + end + end +end diff --git a/spec/workers/use_key_worker_spec.rb b/spec/workers/use_key_worker_spec.rb deleted file mode 100644 index e50c788b82a..00000000000 --- a/spec/workers/use_key_worker_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -require 'spec_helper' - -describe UseKeyWorker do - describe "#perform" do - it "updates the key's last_used_at attribute to the current time when it exists" do - worker = described_class.new - key = create(:key) - current_time = Time.zone.now - - Timecop.freeze(current_time) do - expect { worker.perform(key.id) } - .to change { key.reload.last_used_at }.from(nil).to be_like_time(current_time) - end - end - - it "returns false and skips the job when the key doesn't exist" do - worker = described_class.new - key = create(:key) - - expect(worker.perform(key.id + 1)).to eq false - end - end -end |