diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-06-20 14:10:13 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-06-20 14:10:13 +0300 |
commit | 0ea3fcec397b69815975647f5e2aa5fe944a8486 (patch) | |
tree | 7979381b89d26011bcf9bdc989a40fcc2f1ed4ff /spec/lib/gitlab/sidekiq_middleware/duplicate_jobs | |
parent | 72123183a20411a36d607d70b12d57c484394c8e (diff) |
Add latest changes from gitlab-org/gitlab@15-1-stable-eev15.1.0-rc42
Diffstat (limited to 'spec/lib/gitlab/sidekiq_middleware/duplicate_jobs')
-rw-r--r-- | spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb | 649 | ||||
-rw-r--r-- | spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed_spec.rb | 30 |
2 files changed, 370 insertions, 309 deletions
diff --git a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb index 8d46845548a..d240bf51e67 100644 --- a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gitlab_redis_queues do +RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gitlab_redis_queues, :clean_gitlab_redis_shared_state do using RSpec::Parameterized::TableSyntax subject(:duplicate_job) do @@ -81,135 +81,99 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi end end - describe '#check!' do - context 'when there was no job in the queue yet' do - it { expect(duplicate_job.check!).to eq('123') } + shared_examples 'tracking duplicates in redis' do + describe '#check!' do + context 'when there was no job in the queue yet' do + it { expect(duplicate_job.check!).to eq('123') } - shared_examples 'sets Redis keys with correct TTL' do - it "adds an idempotency key with correct ttl" do - expect { duplicate_job.check! } - .to change { read_idempotency_key_with_ttl(idempotency_key) } - .from([nil, -2]) - .to(['123', be_within(1).of(expected_ttl)]) - end - - context 'when wal locations is not empty' do - it "adds an existing wal locations key with correct ttl" do + shared_examples 'sets Redis keys with correct TTL' do + it "adds an idempotency key with correct ttl" do expect { duplicate_job.check! } - .to change { read_idempotency_key_with_ttl(existing_wal_location_key(idempotency_key, :main)) } - .from([nil, -2]) - .to([wal_locations[:main], be_within(1).of(expected_ttl)]) - .and change { read_idempotency_key_with_ttl(existing_wal_location_key(idempotency_key, :ci)) } + .to change { read_idempotency_key_with_ttl(idempotency_key) } .from([nil, -2]) - .to([wal_locations[:ci], be_within(1).of(expected_ttl)]) + .to(['123', be_within(1).of(expected_ttl)]) end - end - end - context 'with TTL option is not set' do - let(:expected_ttl) { described_class::DEFAULT_DUPLICATE_KEY_TTL } - - it_behaves_like 'sets Redis keys with correct TTL' - end - - context 'when TTL option is set' do - let(:expected_ttl) { 5.minutes } - - before do - allow(duplicate_job).to receive(:options).and_return({ ttl: expected_ttl }) + context 'when wal locations is not empty' do + it "adds an existing wal locations key with correct ttl" do + expect { duplicate_job.check! } + .to change { read_idempotency_key_with_ttl(existing_wal_location_key(idempotency_key, :main)) } + .from([nil, -2]) + .to([wal_locations[:main], be_within(1).of(expected_ttl)]) + .and change { read_idempotency_key_with_ttl(existing_wal_location_key(idempotency_key, :ci)) } + .from([nil, -2]) + .to([wal_locations[:ci], be_within(1).of(expected_ttl)]) + end + end end - it_behaves_like 'sets Redis keys with correct TTL' - end + context 'when TTL option is not set' do + let(:expected_ttl) { described_class::DEFAULT_DUPLICATE_KEY_TTL } - it "adds the idempotency key to the jobs payload" do - expect { duplicate_job.check! }.to change { job['idempotency_key'] }.from(nil).to(idempotency_key) - end - end - - context 'when there was already a job with same arguments in the same queue' do - before do - set_idempotency_key(idempotency_key, 'existing-key') - wal_locations.each do |config_name, location| - set_idempotency_key(existing_wal_location_key(idempotency_key, config_name), location) + it_behaves_like 'sets Redis keys with correct TTL' end - end - it { expect(duplicate_job.check!).to eq('existing-key') } + context 'when TTL option is set' do + let(:expected_ttl) { 5.minutes } - it "does not change the existing key's TTL" do - expect { duplicate_job.check! } - .not_to change { read_idempotency_key_with_ttl(idempotency_key) } - .from(['existing-key', -1]) - end - - it "does not change the existing wal locations key's TTL" do - expect { duplicate_job.check! } - .to not_change { read_idempotency_key_with_ttl(existing_wal_location_key(idempotency_key, :main)) } - .from([wal_locations[:main], -1]) - .and not_change { read_idempotency_key_with_ttl(existing_wal_location_key(idempotency_key, :ci)) } - .from([wal_locations[:ci], -1]) - end + before do + allow(duplicate_job).to receive(:options).and_return({ ttl: expected_ttl }) + end - it 'sets the existing jid' do - duplicate_job.check! + it_behaves_like 'sets Redis keys with correct TTL' + end - expect(duplicate_job.existing_jid).to eq('existing-key') + it "adds the idempotency key to the jobs payload" do + expect { duplicate_job.check! }.to change { job['idempotency_key'] }.from(nil).to(idempotency_key) + end end - end - end - - describe '#update_latest_wal_location!' do - before do - allow(Gitlab::Database).to receive(:database_base_models).and_return( - { main: ::ActiveRecord::Base, - ci: ::ActiveRecord::Base }) - set_idempotency_key(existing_wal_location_key(idempotency_key, :main), existing_wal[:main]) - set_idempotency_key(existing_wal_location_key(idempotency_key, :ci), existing_wal[:ci]) + context 'when there was already a job with same arguments in the same queue' do + before do + set_idempotency_key(idempotency_key, 'existing-key') + wal_locations.each do |config_name, location| + set_idempotency_key(existing_wal_location_key(idempotency_key, config_name), location) + end + end - # read existing_wal_locations - duplicate_job.check! - end + it { expect(duplicate_job.check!).to eq('existing-key') } - context "when the key doesn't exists in redis" do - let(:existing_wal) do - { - main: '0/D525E3A0', - ci: 'AB/12340' - } - end + it "does not change the existing key's TTL" do + expect { duplicate_job.check! } + .not_to change { read_idempotency_key_with_ttl(idempotency_key) } + .from(['existing-key', -1]) + end - let(:new_wal_location_with_offset) do - { - # offset is relative to `existing_wal` - main: ['0/D525E3A8', '8'], - ci: ['AB/12345', '5'] - } - end + it "does not change the existing wal locations key's TTL" do + expect { duplicate_job.check! } + .to not_change { read_idempotency_key_with_ttl(existing_wal_location_key(idempotency_key, :main)) } + .from([wal_locations[:main], -1]) + .and not_change { read_idempotency_key_with_ttl(existing_wal_location_key(idempotency_key, :ci)) } + .from([wal_locations[:ci], -1]) + end - let(:wal_locations) { new_wal_location_with_offset.transform_values(&:first) } + it 'sets the existing jid' do + duplicate_job.check! - it 'stores a wal location to redis with an offset relative to existing wal location' do - expect { duplicate_job.update_latest_wal_location! } - .to change { read_range_from_redis(wal_location_key(idempotency_key, :main)) } - .from([]) - .to(new_wal_location_with_offset[:main]) - .and change { read_range_from_redis(wal_location_key(idempotency_key, :ci)) } - .from([]) - .to(new_wal_location_with_offset[:ci]) + expect(duplicate_job.existing_jid).to eq('existing-key') + end end end - context "when the key exists in redis" do + describe '#update_latest_wal_location!' do before do - rpush_to_redis_key(wal_location_key(idempotency_key, :main), *stored_wal_location_with_offset[:main]) - rpush_to_redis_key(wal_location_key(idempotency_key, :ci), *stored_wal_location_with_offset[:ci]) - end + allow(Gitlab::Database).to receive(:database_base_models).and_return( + { main: ::ActiveRecord::Base, + ci: ::ActiveRecord::Base }) - let(:wal_locations) { new_wal_location_with_offset.transform_values(&:first) } + set_idempotency_key(existing_wal_location_key(idempotency_key, :main), existing_wal[:main]) + set_idempotency_key(existing_wal_location_key(idempotency_key, :ci), existing_wal[:ci]) - context "when the new offset is bigger then the existing one" do + # read existing_wal_locations + duplicate_job.check! + end + + context "when the key doesn't exists in redis" do let(:existing_wal) do { main: '0/D525E3A0', @@ -217,14 +181,6 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi } end - let(:stored_wal_location_with_offset) do - { - # offset is relative to `existing_wal` - main: ['0/D525E3A3', '3'], - ci: ['AB/12342', '2'] - } - end - let(:new_wal_location_with_offset) do { # offset is relative to `existing_wal` @@ -233,154 +189,335 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi } end - it 'updates a wal location to redis with an offset' do + let(:wal_locations) { new_wal_location_with_offset.transform_values(&:first) } + + it 'stores a wal location to redis with an offset relative to existing wal location' do expect { duplicate_job.update_latest_wal_location! } .to change { read_range_from_redis(wal_location_key(idempotency_key, :main)) } - .from(stored_wal_location_with_offset[:main]) + .from([]) .to(new_wal_location_with_offset[:main]) .and change { read_range_from_redis(wal_location_key(idempotency_key, :ci)) } - .from(stored_wal_location_with_offset[:ci]) + .from([]) .to(new_wal_location_with_offset[:ci]) end end - context "when the old offset is not bigger then the existing one" do - let(:existing_wal) do - { - main: '0/D525E3A0', - ci: 'AB/12340' - } + context "when the key exists in redis" do + before do + rpush_to_redis_key(wal_location_key(idempotency_key, :main), *stored_wal_location_with_offset[:main]) + rpush_to_redis_key(wal_location_key(idempotency_key, :ci), *stored_wal_location_with_offset[:ci]) end - let(:stored_wal_location_with_offset) do - { - # offset is relative to `existing_wal` - main: ['0/D525E3A8', '8'], - ci: ['AB/12345', '5'] - } - end + let(:wal_locations) { new_wal_location_with_offset.transform_values(&:first) } - let(:new_wal_location_with_offset) do - { - # offset is relative to `existing_wal` - main: ['0/D525E3A2', '2'], - ci: ['AB/12342', '2'] - } + context "when the new offset is bigger then the existing one" do + let(:existing_wal) do + { + main: '0/D525E3A0', + ci: 'AB/12340' + } + end + + let(:stored_wal_location_with_offset) do + { + # offset is relative to `existing_wal` + main: ['0/D525E3A3', '3'], + ci: ['AB/12342', '2'] + } + end + + let(:new_wal_location_with_offset) do + { + # offset is relative to `existing_wal` + main: ['0/D525E3A8', '8'], + ci: ['AB/12345', '5'] + } + end + + it 'updates a wal location to redis with an offset' do + expect { duplicate_job.update_latest_wal_location! } + .to change { read_range_from_redis(wal_location_key(idempotency_key, :main)) } + .from(stored_wal_location_with_offset[:main]) + .to(new_wal_location_with_offset[:main]) + .and change { read_range_from_redis(wal_location_key(idempotency_key, :ci)) } + .from(stored_wal_location_with_offset[:ci]) + .to(new_wal_location_with_offset[:ci]) + end end - it "does not update a wal location to redis with an offset" do - expect { duplicate_job.update_latest_wal_location! } - .to not_change { read_range_from_redis(wal_location_key(idempotency_key, :main)) } - .from(stored_wal_location_with_offset[:main]) - .and not_change { read_range_from_redis(wal_location_key(idempotency_key, :ci)) } - .from(stored_wal_location_with_offset[:ci]) + context "when the old offset is not bigger then the existing one" do + let(:existing_wal) do + { + main: '0/D525E3A0', + ci: 'AB/12340' + } + end + + let(:stored_wal_location_with_offset) do + { + # offset is relative to `existing_wal` + main: ['0/D525E3A8', '8'], + ci: ['AB/12345', '5'] + } + end + + let(:new_wal_location_with_offset) do + { + # offset is relative to `existing_wal` + main: ['0/D525E3A2', '2'], + ci: ['AB/12342', '2'] + } + end + + it "does not update a wal location to redis with an offset" do + expect { duplicate_job.update_latest_wal_location! } + .to not_change { read_range_from_redis(wal_location_key(idempotency_key, :main)) } + .from(stored_wal_location_with_offset[:main]) + .and not_change { read_range_from_redis(wal_location_key(idempotency_key, :ci)) } + .from(stored_wal_location_with_offset[:ci]) + end end end end - end - describe '#latest_wal_locations' do - context 'when job was deduplicated and wal locations were already persisted' do - before do - rpush_to_redis_key(wal_location_key(idempotency_key, :main), wal_locations[:main], 1024) - rpush_to_redis_key(wal_location_key(idempotency_key, :ci), wal_locations[:ci], 1024) - end + describe '#latest_wal_locations' do + context 'when job was deduplicated and wal locations were already persisted' do + before do + rpush_to_redis_key(wal_location_key(idempotency_key, :main), wal_locations[:main], 1024) + rpush_to_redis_key(wal_location_key(idempotency_key, :ci), wal_locations[:ci], 1024) + end - it { expect(duplicate_job.latest_wal_locations).to eq(wal_locations) } - end + it { expect(duplicate_job.latest_wal_locations).to eq(wal_locations) } + end - context 'when job is not deduplication and wal locations were not persisted' do - it { expect(duplicate_job.latest_wal_locations).to be_empty } + context 'when job is not deduplication and wal locations were not persisted' do + it { expect(duplicate_job.latest_wal_locations).to be_empty } + end end - end - describe '#delete!' do - context "when we didn't track the definition" do - it { expect { duplicate_job.delete! }.not_to raise_error } - end + describe '#delete!' do + context "when we didn't track the definition" do + it { expect { duplicate_job.delete! }.not_to raise_error } + end - context 'when the key exists in redis' do - before do - set_idempotency_key(idempotency_key, 'existing-jid') - set_idempotency_key(deduplicated_flag_key, 1) - wal_locations.each do |config_name, location| - set_idempotency_key(existing_wal_location_key(idempotency_key, config_name), location) - set_idempotency_key(wal_location_key(idempotency_key, config_name), location) + context 'when the key exists in redis' do + before do + set_idempotency_key(idempotency_key, 'existing-jid') + set_idempotency_key(deduplicated_flag_key, 1) + wal_locations.each do |config_name, location| + set_idempotency_key(existing_wal_location_key(idempotency_key, config_name), location) + set_idempotency_key(wal_location_key(idempotency_key, config_name), location) + end end - end - shared_examples 'deleting the duplicate job' do - shared_examples 'deleting keys from redis' do |key_name| - it "removes the #{key_name} from redis" do - expect { duplicate_job.delete! } - .to change { read_idempotency_key_with_ttl(key) } - .from([from_value, -1]) - .to([nil, -2]) + shared_examples 'deleting the duplicate job' do + shared_examples 'deleting keys from redis' do |key_name| + it "removes the #{key_name} from redis" do + expect { duplicate_job.delete! } + .to change { read_idempotency_key_with_ttl(key) } + .from([from_value, -1]) + .to([nil, -2]) + end + end + + shared_examples 'does not delete key from redis' do |key_name| + it "does not remove the #{key_name} from redis" do + expect { duplicate_job.delete! } + .to not_change { read_idempotency_key_with_ttl(key) } + .from([from_value, -1]) + end + end + + it_behaves_like 'deleting keys from redis', 'idempotent key' do + let(:key) { idempotency_key } + let(:from_value) { 'existing-jid' } + end + + it_behaves_like 'deleting keys from redis', 'deduplication counter key' do + let(:key) { deduplicated_flag_key } + let(:from_value) { '1' } + end + + it_behaves_like 'deleting keys from redis', 'existing wal location keys for main database' do + let(:key) { existing_wal_location_key(idempotency_key, :main) } + let(:from_value) { wal_locations[:main] } + end + + it_behaves_like 'deleting keys from redis', 'existing wal location keys for ci database' do + let(:key) { existing_wal_location_key(idempotency_key, :ci) } + let(:from_value) { wal_locations[:ci] } + end + + it_behaves_like 'deleting keys from redis', 'latest wal location keys for main database' do + let(:key) { wal_location_key(idempotency_key, :main) } + let(:from_value) { wal_locations[:main] } + end + + it_behaves_like 'deleting keys from redis', 'latest wal location keys for ci database' do + let(:key) { wal_location_key(idempotency_key, :ci) } + let(:from_value) { wal_locations[:ci] } end end - shared_examples 'does not delete key from redis' do |key_name| - it "does not remove the #{key_name} from redis" do - expect { duplicate_job.delete! } - .to not_change { read_idempotency_key_with_ttl(key) } - .from([from_value, -1]) + context 'when the idempotency key is not part of the job' do + it_behaves_like 'deleting the duplicate job' + + it 'recalculates the idempotency hash' do + expect(duplicate_job).to receive(:idempotency_hash).and_call_original + + duplicate_job.delete! end end - it_behaves_like 'deleting keys from redis', 'idempotent key' do - let(:key) { idempotency_key } - let(:from_value) { 'existing-jid' } + context 'when the idempotency key is part of the job' do + let(:idempotency_key) { 'not the same as what we calculate' } + let(:job) { super().merge('idempotency_key' => idempotency_key) } + + it_behaves_like 'deleting the duplicate job' + + it 'does not recalculate the idempotency hash' do + expect(duplicate_job).not_to receive(:idempotency_hash) + + duplicate_job.delete! + end end + end + end - it_behaves_like 'deleting keys from redis', 'deduplication counter key' do - let(:key) { deduplicated_flag_key } - let(:from_value) { '1' } + describe '#set_deduplicated_flag!' do + context 'when the job is reschedulable' do + before do + allow(duplicate_job).to receive(:reschedulable?) { true } end - it_behaves_like 'deleting keys from redis', 'existing wal location keys for main database' do - let(:key) { existing_wal_location_key(idempotency_key, :main) } - let(:from_value) { wal_locations[:main] } + it 'sets the key in Redis' do + duplicate_job.set_deduplicated_flag! + + flag = with_redis { |redis| redis.get(deduplicated_flag_key) } + + expect(flag).to eq(described_class::DEDUPLICATED_FLAG_VALUE.to_s) end - it_behaves_like 'deleting keys from redis', 'existing wal location keys for ci database' do - let(:key) { existing_wal_location_key(idempotency_key, :ci) } - let(:from_value) { wal_locations[:ci] } + it 'sets, gets and cleans up the deduplicated flag' do + expect(duplicate_job.should_reschedule?).to eq(false) + + duplicate_job.set_deduplicated_flag! + expect(duplicate_job.should_reschedule?).to eq(true) + + duplicate_job.delete! + expect(duplicate_job.should_reschedule?).to eq(false) end + end - it_behaves_like 'deleting keys from redis', 'latest wal location keys for main database' do - let(:key) { wal_location_key(idempotency_key, :main) } - let(:from_value) { wal_locations[:main] } + context 'when the job is not reschedulable' do + before do + allow(duplicate_job).to receive(:reschedulable?) { false } end - it_behaves_like 'deleting keys from redis', 'latest wal location keys for ci database' do - let(:key) { wal_location_key(idempotency_key, :ci) } - let(:from_value) { wal_locations[:ci] } + it 'does not set the key in Redis' do + duplicate_job.set_deduplicated_flag! + + flag = with_redis { |redis| redis.get(deduplicated_flag_key) } + + expect(flag).to be_nil end - end - context 'when the idempotency key is not part of the job' do - it_behaves_like 'deleting the duplicate job' + it 'does not set the deduplicated flag' do + expect(duplicate_job.should_reschedule?).to eq(false) - it 'recalculates the idempotency hash' do - expect(duplicate_job).to receive(:idempotency_hash).and_call_original + duplicate_job.set_deduplicated_flag! + expect(duplicate_job.should_reschedule?).to eq(false) duplicate_job.delete! + expect(duplicate_job.should_reschedule?).to eq(false) end end + end + + describe '#duplicate?' do + it "raises an error if the check wasn't performed" do + expect { duplicate_job.duplicate? }.to raise_error /Call `#check!` first/ + end - context 'when the idempotency key is part of the job' do - let(:idempotency_key) { 'not the same as what we calculate' } - let(:job) { super().merge('idempotency_key' => idempotency_key) } + it 'returns false if the existing jid equals the job jid' do + duplicate_job.check! - it_behaves_like 'deleting the duplicate job' + expect(duplicate_job.duplicate?).to be(false) + end - it 'does not recalculate the idempotency hash' do - expect(duplicate_job).not_to receive(:idempotency_hash) + it 'returns false if the existing jid is different from the job jid' do + set_idempotency_key(idempotency_key, 'a different jid') + duplicate_job.check! - duplicate_job.delete! + expect(duplicate_job.duplicate?).to be(true) + end + end + + def existing_wal_location_key(idempotency_key, connection_name) + "#{idempotency_key}:#{connection_name}:existing_wal_location" + end + + def wal_location_key(idempotency_key, connection_name) + "#{idempotency_key}:#{connection_name}:wal_location" + end + + def set_idempotency_key(key, value = '1') + with_redis { |r| r.set(key, value) } + end + + def rpush_to_redis_key(key, wal, offset) + with_redis { |r| r.rpush(key, [wal, offset]) } + end + + def read_idempotency_key_with_ttl(key) + with_redis do |redis| + redis.pipelined do |p| + p.get(key) + p.ttl(key) end end end + + def read_range_from_redis(key) + with_redis do |redis| + redis.lrange(key, 0, -1) + end + end + end + + context 'with multi-store feature flags turned on' do + def with_redis(&block) + Gitlab::Redis::DuplicateJobs.with(&block) + end + + it 'use Gitlab::Redis::DuplicateJobs.with' do + expect(Gitlab::Redis::DuplicateJobs).to receive(:with).and_call_original + expect(Sidekiq).not_to receive(:redis) + + duplicate_job.check! + end + + it_behaves_like 'tracking duplicates in redis' + end + + context 'when both multi-store feature flags are off' do + def with_redis(&block) + Sidekiq.redis(&block) + end + + before do + stub_feature_flags(use_primary_and_secondary_stores_for_duplicate_jobs: false) + stub_feature_flags(use_primary_store_as_default_for_duplicate_jobs: false) + end + + it 'use Sidekiq.redis' do + expect(Sidekiq).to receive(:redis).and_call_original + expect(Gitlab::Redis::DuplicateJobs).not_to receive(:with) + + duplicate_job.check! + end + + it_behaves_like 'tracking duplicates in redis' end describe '#scheduled?' do @@ -449,75 +586,6 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi end end - describe '#set_deduplicated_flag!' do - context 'when the job is reschedulable' do - before do - allow(duplicate_job).to receive(:reschedulable?) { true } - end - - it 'sets the key in Redis' do - duplicate_job.set_deduplicated_flag! - - flag = Sidekiq.redis { |redis| redis.get(deduplicated_flag_key) } - - expect(flag).to eq(described_class::DEDUPLICATED_FLAG_VALUE.to_s) - end - - it 'sets, gets and cleans up the deduplicated flag' do - expect(duplicate_job.should_reschedule?).to eq(false) - - duplicate_job.set_deduplicated_flag! - expect(duplicate_job.should_reschedule?).to eq(true) - - duplicate_job.delete! - expect(duplicate_job.should_reschedule?).to eq(false) - end - end - - context 'when the job is not reschedulable' do - before do - allow(duplicate_job).to receive(:reschedulable?) { false } - end - - it 'does not set the key in Redis' do - duplicate_job.set_deduplicated_flag! - - flag = Sidekiq.redis { |redis| redis.get(deduplicated_flag_key) } - - expect(flag).to be_nil - end - - it 'does not set the deduplicated flag' do - expect(duplicate_job.should_reschedule?).to eq(false) - - duplicate_job.set_deduplicated_flag! - expect(duplicate_job.should_reschedule?).to eq(false) - - duplicate_job.delete! - expect(duplicate_job.should_reschedule?).to eq(false) - end - end - end - - describe '#duplicate?' do - it "raises an error if the check wasn't performed" do - expect { duplicate_job.duplicate? }.to raise_error /Call `#check!` first/ - end - - it 'returns false if the existing jid equals the job jid' do - duplicate_job.check! - - expect(duplicate_job.duplicate?).to be(false) - end - - it 'returns false if the existing jid is different from the job jid' do - set_idempotency_key(idempotency_key, 'a different jid') - duplicate_job.check! - - expect(duplicate_job.duplicate?).to be(true) - end - end - describe '#scheduled_at' do let(:scheduled_at) { 42 } let(:job) do @@ -592,35 +660,4 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi end end end - - def existing_wal_location_key(idempotency_key, connection_name) - "#{idempotency_key}:#{connection_name}:existing_wal_location" - end - - def wal_location_key(idempotency_key, connection_name) - "#{idempotency_key}:#{connection_name}:wal_location" - end - - def set_idempotency_key(key, value = '1') - Sidekiq.redis { |r| r.set(key, value) } - end - - def rpush_to_redis_key(key, wal, offset) - Sidekiq.redis { |r| r.rpush(key, [wal, offset]) } - end - - def read_idempotency_key_with_ttl(key) - Sidekiq.redis do |redis| - redis.pipelined do |p| - p.get(key) - p.ttl(key) - end - end - end - - def read_range_from_redis(key) - Sidekiq.redis do |redis| - redis.lrange(key, 0, -1) - end - end end diff --git a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed_spec.rb b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed_spec.rb index 963301bc001..ab9d14ad729 100644 --- a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/strategies/until_executed_spec.rb @@ -23,8 +23,15 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::Strategies::UntilExecut end end + it 'deletes the lock even if an error occurs' do + expect(fake_duplicate_job).not_to receive(:scheduled?) + expect(fake_duplicate_job).to receive(:delete!).once + + perform_strategy_with_error + end + it 'does not reschedule the job even if deduplication happened' do - expect(fake_duplicate_job).to receive(:delete!) + expect(fake_duplicate_job).to receive(:delete!).once expect(fake_duplicate_job).not_to receive(:reschedule) strategy.perform({}) do @@ -33,16 +40,33 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::Strategies::UntilExecut end context 'when job is reschedulable' do - it 'reschedules the job if deduplication happened' do + before do allow(fake_duplicate_job).to receive(:should_reschedule?) { true } + end - expect(fake_duplicate_job).to receive(:delete!) + it 'reschedules the job if deduplication happened' do + expect(fake_duplicate_job).to receive(:delete!).once expect(fake_duplicate_job).to receive(:reschedule).once strategy.perform({}) do proc.call end end + + it 'does not reschedule the job if an error occurs' do + expect(fake_duplicate_job).to receive(:delete!).once + expect(fake_duplicate_job).not_to receive(:reschedule) + + perform_strategy_with_error + end + end + + def perform_strategy_with_error + expect do + strategy.perform({}) do + raise 'expected error' + end + end.to raise_error(RuntimeError, 'expected error') end end end |