Permalink
Browse files

Merge branch 'remove-use-key-worker' into 'master'

Stop using Sidekiq for updating Key#last_used_at

Closes #36663

See merge request gitlab-org/gitlab-ce!14391
  • Loading branch information...
rymai committed Sep 21, 2017
2 parents 6374530 + b3566a0 commit d863326ea8a184d012e81ab47fb5e4e62801a0c6
View
@@ -4,8 +4,6 @@ class Key < ActiveRecord::Base
include Gitlab::CurrentSettings
include Sortable
LAST_USED_AT_REFRESH_TIME = 1.day.to_i
belongs_to :user
before_validation :generate_fingerprint
@@ -54,10 +52,7 @@ def shell_id
end
def update_last_used_at
lease = Gitlab::ExclusiveLease.new("key_update_last_used_at:#{id}", timeout: LAST_USED_AT_REFRESH_TIME)
return unless lease.try_obtain
UseKeyWorker.perform_async(id)
Keys::LastUsedService.new(self).execute
end
def add_to_shell
@@ -0,0 +1,33 @@
module Keys
class LastUsedService
TIMEOUT = 1.day.to_i
attr_reader :key
# key - The Key for which to update the last used timestamp.
def initialize(key)
@key = key
end
def execute
# We _only_ want to update last_used_at and not also updated_at (which
# would be updated when using #touch).
key.update_column(:last_used_at, Time.zone.now) if update?
end
def update?
last_used = key.last_used_at
return false if last_used && (Time.zone.now - last_used) <= TIMEOUT
!!redis_lease.try_obtain
end
private
def redis_lease
Gitlab::ExclusiveLease
.new("key_update_last_used_at:#{key.id}", timeout: TIMEOUT)
end
end
end

This file was deleted.

Oops, something went wrong.
@@ -0,0 +1,5 @@
---
title: Stop using Sidekiq for updating Key#last_used_at
merge_request:
author:
type: changed
@@ -38,7 +38,6 @@
- [invalid_gpg_signature_update, 2]
- [create_gpg_signature, 2]
- [upload_checksum, 1]
- [use_key, 1]
- [repository_fork, 1]
- [repository_import, 1]
- [project_service, 1]
View
@@ -37,30 +37,17 @@
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
@@ -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

This file was deleted.

Oops, something went wrong.

0 comments on commit d863326

Please sign in to comment.