Commit b5afee0a authored by Sean McGivern's avatar Sean McGivern

Remove implicit job deduplication feature flags

We added these as a safety net, but:

1. We've never used them.
2. There's an implicit feature flag per idempotent worker.
3. Idempotency and job deduplication are now well established in our
   codebase.

So we don't need them any more.
parent 2fdd4ce2
......@@ -251,26 +251,6 @@ module AuthorizedProjectUpdate
end
```
#### Troubleshooting
If the automatic deduplication were to cause issues in certain
queues. This can be temporarily disabled by enabling a feature flag
named `disable_<queue name>_deduplication`. For example to disable
deduplication for the `AuthorizedProjectsWorker`, we would enable the
feature flag `disable_authorized_projects_deduplication`.
From ChatOps:
```shell
/chatops run feature set disable_authorized_projects_deduplication true
```
From the rails console:
```ruby
Feature.enable!(:disable_authorized_projects_deduplication)
```
## Limited capacity worker
It is possible to limit the number of concurrent running jobs for a worker class
......
......@@ -70,10 +70,6 @@ module Gitlab
jid != existing_jid
end
def droppable?
idempotent? && ::Feature.disabled?("disable_#{queue_name}_deduplication", type: :ops)
end
def scheduled_at
job['at']
end
......@@ -85,6 +81,13 @@ module Gitlab
worker_klass.get_deduplication_options
end
def idempotent?
return false unless worker_klass
return false unless worker_klass.respond_to?(:idempotent?)
worker_klass.idempotent?
end
private
attr_reader :queue_name, :job
......@@ -128,13 +131,6 @@ module Gitlab
def idempotency_string
"#{worker_class_name}:#{arguments.join('-')}"
end
def idempotent?
return false unless worker_klass
return false unless worker_klass.respond_to?(:idempotent?)
worker_klass.idempotent?
end
end
end
end
......
......@@ -13,7 +13,7 @@ module Gitlab
if deduplicatable_job? && check! && duplicate_job.duplicate?
job['duplicate-of'] = duplicate_job.existing_jid
if duplicate_job.droppable?
if duplicate_job.idempotent?
Gitlab::SidekiqLogging::DeduplicationLogger.instance.log(
job, "dropped #{strategy_name}", duplicate_job.options)
return false
......
......@@ -131,31 +131,6 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi
end
end
describe '#droppable?' do
where(:idempotent, :prevent_deduplication) do
# [true, false].repeated_permutation(2)
[[true, true],
[true, false],
[false, true],
[false, false]]
end
with_them do
before do
allow(AuthorizedProjectsWorker).to receive(:idempotent?).and_return(idempotent)
stub_feature_flags("disable_#{queue}_deduplication" => prevent_deduplication)
end
it 'is droppable when all conditions are met' do
if idempotent && !prevent_deduplication
expect(duplicate_job).to be_droppable
else
expect(duplicate_job).not_to be_droppable
end
end
end
end
describe '#scheduled_at' do
let(:scheduled_at) { 42 }
let(:job) do
......@@ -181,6 +156,46 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi
end
end
describe '#idempotent?' do
context 'when worker class does not exist' do
let(:job) { { 'class' => '' } }
it 'returns false' do
expect(duplicate_job).not_to be_idempotent
end
end
context 'when worker class does not respond to #idempotent?' do
before do
stub_const('AuthorizedProjectsWorker', Class.new)
end
it 'returns false' do
expect(duplicate_job).not_to be_idempotent
end
end
context 'when worker class is not idempotent' do
before do
allow(AuthorizedProjectsWorker).to receive(:idempotent?).and_return(false)
end
it 'returns false' do
expect(duplicate_job).not_to be_idempotent
end
end
context 'when worker class is idempotent' do
before do
allow(AuthorizedProjectsWorker).to receive(:idempotent?).and_return(true)
end
it 'returns true' do
expect(duplicate_job).to be_idempotent
end
end
end
def set_idempotency_key(key, value = '1')
Sidekiq.redis { |r| r.set(key, value) }
end
......
......@@ -38,7 +38,7 @@ RSpec.shared_examples 'deduplicating jobs when scheduling' do |strategy_name|
it 'adds the jid of the existing job to the job hash' do
allow(fake_duplicate_job).to receive(:scheduled?).and_return(false)
allow(fake_duplicate_job).to receive(:check!).and_return('the jid')
allow(fake_duplicate_job).to receive(:droppable?).and_return(true)
allow(fake_duplicate_job).to receive(:idempotent?).and_return(true)
allow(fake_duplicate_job).to receive(:options).and_return({})
job_hash = {}
......@@ -62,7 +62,7 @@ RSpec.shared_examples 'deduplicating jobs when scheduling' do |strategy_name|
receive(:check!)
.with(Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob::DUPLICATE_KEY_TTL)
.and_return('the jid'))
allow(fake_duplicate_job).to receive(:droppable?).and_return(true)
allow(fake_duplicate_job).to receive(:idempotent?).and_return(true)
job_hash = {}
expect(fake_duplicate_job).to receive(:duplicate?).and_return(true)
......@@ -82,7 +82,7 @@ RSpec.shared_examples 'deduplicating jobs when scheduling' do |strategy_name|
allow(fake_duplicate_job).to receive(:options).and_return({ including_scheduled: true })
allow(fake_duplicate_job).to(
receive(:check!).with(time_diff.to_i).and_return('the jid'))
allow(fake_duplicate_job).to receive(:droppable?).and_return(true)
allow(fake_duplicate_job).to receive(:idempotent?).and_return(true)
job_hash = {}
expect(fake_duplicate_job).to receive(:duplicate?).and_return(true)
......@@ -104,13 +104,13 @@ RSpec.shared_examples 'deduplicating jobs when scheduling' do |strategy_name|
allow(fake_duplicate_job).to receive(:duplicate?).and_return(true)
allow(fake_duplicate_job).to receive(:options).and_return({})
allow(fake_duplicate_job).to receive(:existing_jid).and_return('the jid')
allow(fake_duplicate_job).to receive(:droppable?).and_return(true)
allow(fake_duplicate_job).to receive(:idempotent?).and_return(true)
end
it 'drops the job' do
schedule_result = nil
expect(fake_duplicate_job).to receive(:droppable?).and_return(true)
expect(fake_duplicate_job).to receive(:idempotent?).and_return(true)
expect { |b| schedule_result = strategy.schedule({}, &b) }.not_to yield_control
expect(schedule_result).to be(false)
......
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