diff --git a/app/models/concerns/cells/transaction_callback.rb b/app/models/concerns/cells/transaction_callback.rb index 84264115eeebe4fcd631b7ea7b91df0b2983503d..79c2a25ec281bb84fa64aef5b18a9dfe2f2c8c70 100644 --- a/app/models/concerns/cells/transaction_callback.rb +++ b/app/models/concerns/cells/transaction_callback.rb @@ -2,6 +2,7 @@ module Cells module TransactionCallback + TOPOLOGY_SERVICE_TIMEOUT_IN_TRANSACTION_IN_SECONDS = 0.2 THREAD_VARIABLE_NAME = :cells_unique_transaction_callback_visited_stack def self.visited @@ -39,7 +40,10 @@ def commit_db_transaction # and when that happens, we'll roll it back, and we need to look at # the stack to know if it's a unbalanced ROLLBACK or not, therefore # we only pop after successful COMMIT or ROLLBACk. - Unique.claim_and_commit(TransactionCallback.last_visited.values) do + changes = TransactionCallback.last_visited.values + + Unique.claim_and_commit(changes, + timeout: TOPOLOGY_SERVICE_TIMEOUT_IN_TRANSACTION_IN_SECONDS) do super TransactionCallback.visited_stack.pop end @@ -48,7 +52,8 @@ def commit_db_transaction def rollback_db_transaction return super unless Current.feature_cells_unique_claims - Unique.rollback do + Unique.rollback( + timeout: TOPOLOGY_SERVICE_TIMEOUT_IN_TRANSACTION_IN_SECONDS) do super # Rails can be issuing unbalanced ROLLBACK (no corresponding BEGIN), diff --git a/app/models/concerns/cells/unique.rb b/app/models/concerns/cells/unique.rb index 1c6a5c86b2bb28bfbe373670c6665c9b955f9427..b3d8abe57dcdb1c81dfaaececf96e55c41ae4c5c 100644 --- a/app/models/concerns/cells/unique.rb +++ b/app/models/concerns/cells/unique.rb @@ -61,22 +61,24 @@ def cells_unique_has_many(name) end end - def self.claim_and_commit(changes) + def self.claim_and_commit(changes, timeout: nil) if changes.empty? yield if block_given? else pp changes + deadline = Time.now.to_f + timeout if timeout claim_client = Gitlab::TopologyServiceClient::ClaimService.new - claim_client.claim_and_commit(changes) do + claim_client.claim_and_commit(changes, deadline: deadline) do yield if block_given? end end end - def self.rollback + def self.rollback(timeout: nil) + deadline = Time.now.to_f + timeout if timeout claim_client = Gitlab::TopologyServiceClient::ClaimService.new - claim_client.rollback do + claim_client.rollback(deadline: deadline) do yield end end diff --git a/lib/gitlab/topology_service_client/claim_service.rb b/lib/gitlab/topology_service_client/claim_service.rb index 3380f10819df02616a887477c2231e669d34ea0f..ff2ea8b3cae0224eefac1ab74be866bb1939c30f 100644 --- a/lib/gitlab/topology_service_client/claim_service.rb +++ b/lib/gitlab/topology_service_client/claim_service.rb @@ -13,15 +13,15 @@ def self.set_last_outstanding_lease_uuid(uuid) Thread.current.thread_variable_set(THREAD_VARIABLE_NAME, uuid) end - def claim_and_commit(changes) - lease = claim(changes) + def claim_and_commit(changes, **request_options) + lease = claim(changes, **request_options) yield - commit(lease) + commit(lease, **request_options) end - def rollback + def rollback(**request_options) # See why we're setting it to nil immediately in #commit lease_uuid = self.class.last_outstanding_lease_uuid self.class.set_last_outstanding_lease_uuid(nil) @@ -39,12 +39,13 @@ def rollback Gitlab::Cells::TopologyService::Claims::V1::RollbackUpdateRequest.new( lease_uuid: build_claim_lease(lease_uuid), cell_id: cell_id - ) + ), + **request_options ) end end - def commit(lease) + def commit(lease, **request_options) # Always set to nil even when we fail to commit, because when we're # calling this, we know we already committed to local database, and # in that case, this is considered done and we should move on. This @@ -63,7 +64,8 @@ def commit(lease) Gitlab::Cells::TopologyService::Claims::V1::CommitUpdateRequest.new( lease_uuid: build_claim_lease(lease.uuid), cell_id: cell_id - ) + ), + **request_options ) # Test error here: @@ -74,11 +76,12 @@ def commit(lease) private - def claim(changes) + def claim(changes, **request_options) # Test error here: # raise "Test error while BeginUpdate" - response = client.begin_update(build_claim_request(changes)) + response = client.begin_update( + build_claim_request(changes), **request_options) # Test error here: # raise "Test error after BeginUpdate before creating local lease" diff --git a/spec/models/concerns/cells/transaction_callback_spec.rb b/spec/models/concerns/cells/transaction_callback_spec.rb index bf1fa134551f82066128b7462d6740590df86887..2ce8bee8fcf70642a98ba2039fa5ff3640b1b4e5 100644 --- a/spec/models/concerns/cells/transaction_callback_spec.rb +++ b/spec/models/concerns/cells/transaction_callback_spec.rb @@ -102,7 +102,9 @@ def rollback_db_transaction it 'claims and commits with visited data' do expect(Cells::Unique).to receive(:claim_and_commit) - .with([{ 'email' => %w[old new] }]) + .with( + [{ 'email' => %w[old new] }], + timeout: described_class::TOPOLOGY_SERVICE_TIMEOUT_IN_TRANSACTION_IN_SECONDS) .and_yield connection.commit_db_transaction @@ -132,7 +134,11 @@ def rollback_db_transaction it 'handles empty visited data' do described_class.visited_stack.last.clear - expect(Cells::Unique).to receive(:claim_and_commit).with([]).and_yield + expect(Cells::Unique).to receive(:claim_and_commit) + .with( + [], + timeout: described_class::TOPOLOGY_SERVICE_TIMEOUT_IN_TRANSACTION_IN_SECONDS) + .and_yield connection.commit_db_transaction expect(described_class.visited_stack).to be_empty @@ -196,7 +202,9 @@ def rollback_db_transaction described_class.visited[['users', 1]] = { 'name' => %w[old new] } expect(Cells::Unique).to receive(:claim_and_commit) - .with([{ 'name' => %w[old new] }]) + .with( + [{ 'name' => %w[old new] }], + timeout: described_class::TOPOLOGY_SERVICE_TIMEOUT_IN_TRANSACTION_IN_SECONDS) .and_yield connection.commit_db_transaction @@ -227,7 +235,9 @@ def rollback_db_transaction connection.rollback_db_transaction expect(Cells::Unique).to receive(:claim_and_commit) - .with(['data']) + .with( + ['data'], + timeout: described_class::TOPOLOGY_SERVICE_TIMEOUT_IN_TRANSACTION_IN_SECONDS) .and_yield connection.commit_db_transaction diff --git a/spec/models/concerns/cells/unique_spec.rb b/spec/models/concerns/cells/unique_spec.rb index a3a95fa7d0a0bf392d41eb883eea344507f26440..7d57fe5ba84a3f2fa9fb0c4a94aa2c5f8a187a54 100644 --- a/spec/models/concerns/cells/unique_spec.rb +++ b/spec/models/concerns/cells/unique_spec.rb @@ -35,10 +35,12 @@ end it 'calls claim service when changes exist' do - changes = { ['users', 1] => { 'email' => %w[old new] } } - expect(claim_service).to receive(:claim_and_commit).with(changes) + freeze_time do + changes = { ['users', 1] => { 'email' => %w[old new] } } + expect(claim_service).to receive(:claim_and_commit).with(changes, deadline: Time.now.to_f + 0.1) - described_class.claim_and_commit(changes) + described_class.claim_and_commit(changes, timeout: 0.1) + end end end