From 057351ae060cf9ed53632ed56b4f9bfb712e7e9f Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Tue, 30 Sep 2025 15:00:11 +0200 Subject: [PATCH 1/8] Adds client timeout to Topology Service Client Changelog: changed --- .../topology_service_client/base_service.rb | 9 +++- .../base_service_spec.rb | 53 ++++++++++++++++++- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/topology_service_client/base_service.rb b/lib/gitlab/topology_service_client/base_service.rb index febd8950d48efb..27d59917a3fa24 100644 --- a/lib/gitlab/topology_service_client/base_service.rb +++ b/lib/gitlab/topology_service_client/base_service.rb @@ -2,6 +2,8 @@ module Gitlab module TopologyServiceClient + DEFAULT_TIMEOUT = 10.seconds + class BaseService def initialize raise NotImplementedError unless enabled? @@ -13,10 +15,15 @@ def client @client ||= service_class.new( topology_service_address, service_credentials, - interceptors: [MetadataInterceptor.new] + interceptors: [MetadataInterceptor.new], + **options ) end + def options + { timeout: Time.current + DEFAULT_TIMEOUT } + end + def cell_id @cell_id ||= Gitlab.config.cell.id end diff --git a/spec/lib/gitlab/topology_service_client/base_service_spec.rb b/spec/lib/gitlab/topology_service_client/base_service_spec.rb index 34683edf2928e5..2c1caced2b1efa 100644 --- a/spec/lib/gitlab/topology_service_client/base_service_spec.rb +++ b/spec/lib/gitlab/topology_service_client/base_service_spec.rb @@ -172,6 +172,9 @@ end describe '#client' do + let(:mock_service_class) { instance_double(Class) } + let(:mock_service) { instance_double(Class, new: instance_double(Object)) } + before do stub_config(cell: { enabled: true, @@ -180,15 +183,63 @@ tls: { enabled: false } } }) + + allow(base_service).to receive(:service_class).and_return(mock_service_class) end it 'includes MetadataInterceptor in client initialization' do expect(Gitlab::TopologyServiceClient::MetadataInterceptor).to receive(:new) - mock_service = instance_double(Class, new: instance_double(Object)) allow(base_service).to receive(:service_class).and_return(mock_service) base_service.send(:client) end + + it 'includes timeout in client initialization' do + freeze_time do + expected_deadline = Time.current + Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT + + expect(mock_service_class).to receive(:new).with('test:50051', :this_channel_is_insecure, + hash_including( + interceptors: array_including(instance_of(Gitlab::TopologyServiceClient::MetadataInterceptor)), + timeout: expected_deadline + ) + ).and_return(mock_service) + + base_service.send(:client) + end + end + + it 'uses the default timeout constant' do + freeze_time do + expected_deadline = Time.current + 10.seconds + + expect(mock_service_class).to receive(:new).with(anything, anything, + hash_including(timeout: expected_deadline)).and_return(mock_service) + + base_service.send(:client) + end + end + end + + describe '#options' do + before do + stub_config(cell: { + enabled: true, + topology_service_client: { + address: 'test:50051', + tls: { enabled: false } + } + }) + end + + it 'returns a hash with timeout set to DEFAULT_TIMEOUT from now' do + freeze_time do + expected_deadline = Time.current + Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT + options = base_service.send(:options) + + expect(options).to eq({ timeout: expected_deadline }) + end + end end end -- GitLab From e78f31fa8d3e4455cbaa08b39082230d6ab7a66c Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Tue, 30 Sep 2025 15:23:37 +0200 Subject: [PATCH 2/8] Adds configurable TS timeout --- config/initializers/1_settings.rb | 1 + lib/gitlab/topology_service_client/base_service.rb | 8 ++++++-- .../topology_service_client/base_service_spec.rb | 13 ++++++++----- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 1c3ebd22e3f4ea..ebe5747ac46bfd 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -1165,6 +1165,7 @@ Settings.cell.topology_service_client['ca_file'] ||= nil Settings.cell.topology_service_client['certificate_file'] ||= nil Settings.cell.topology_service_client['private_key_file'] ||= nil +Settings.cell.topology_service_client['timeout'] ||= nil Settings.cell.topology_service_client['tls'] ||= {} Settings.cell.topology_service_client['tls']['enabled'] = true if Settings.cell.topology_service_client['tls']['enabled'].nil? Settings.cell.topology_service_client['metadata'] ||= {} diff --git a/lib/gitlab/topology_service_client/base_service.rb b/lib/gitlab/topology_service_client/base_service.rb index 27d59917a3fa24..7d5ce47aedcf28 100644 --- a/lib/gitlab/topology_service_client/base_service.rb +++ b/lib/gitlab/topology_service_client/base_service.rb @@ -2,7 +2,7 @@ module Gitlab module TopologyServiceClient - DEFAULT_TIMEOUT = 10.seconds + DEFAULT_TIMEOUT = 0.2.seconds class BaseService def initialize @@ -21,7 +21,11 @@ def client end def options - { timeout: Time.current + DEFAULT_TIMEOUT } + { timeout: Time.current + default_timeout } + end + + def default_timeout + topology_service_config.timeout || DEFAULT_TIMEOUT end def cell_id diff --git a/spec/lib/gitlab/topology_service_client/base_service_spec.rb b/spec/lib/gitlab/topology_service_client/base_service_spec.rb index 2c1caced2b1efa..e74a6d0fd268b2 100644 --- a/spec/lib/gitlab/topology_service_client/base_service_spec.rb +++ b/spec/lib/gitlab/topology_service_client/base_service_spec.rb @@ -174,13 +174,15 @@ describe '#client' do let(:mock_service_class) { instance_double(Class) } let(:mock_service) { instance_double(Class, new: instance_double(Object)) } + let(:timeout) { 0.5.seconds } before do stub_config(cell: { enabled: true, topology_service_client: { address: 'test:50051', - tls: { enabled: false } + tls: { enabled: false }, + timeout: timeout } }) @@ -197,7 +199,7 @@ it 'includes timeout in client initialization' do freeze_time do - expected_deadline = Time.current + Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT + expected_deadline = Time.current + timeout expect(mock_service_class).to receive(:new).with('test:50051', :this_channel_is_insecure, hash_including( @@ -210,9 +212,9 @@ end end - it 'uses the default timeout constant' do + it 'uses the timeout constant' do freeze_time do - expected_deadline = Time.current + 10.seconds + expected_deadline = Time.current + timeout expect(mock_service_class).to receive(:new).with(anything, anything, hash_including(timeout: expected_deadline)).and_return(mock_service) @@ -228,7 +230,8 @@ enabled: true, topology_service_client: { address: 'test:50051', - tls: { enabled: false } + tls: { enabled: false }, + timeout: nil } }) end -- GitLab From f3fda69d2d347a458f94dccd74990125f1c7981f Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Wed, 1 Oct 2025 10:14:13 +0200 Subject: [PATCH 3/8] Apply reviewer's feedback --- lib/gitlab/topology_service_client/base_service.rb | 8 +++++--- .../gitlab/topology_service_client/base_service_spec.rb | 6 +++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/gitlab/topology_service_client/base_service.rb b/lib/gitlab/topology_service_client/base_service.rb index 7d5ce47aedcf28..3e86dd36286f29 100644 --- a/lib/gitlab/topology_service_client/base_service.rb +++ b/lib/gitlab/topology_service_client/base_service.rb @@ -5,8 +5,10 @@ module TopologyServiceClient DEFAULT_TIMEOUT = 0.2.seconds class BaseService - def initialize + def initialize(timeout: nil) raise NotImplementedError unless enabled? + + @timeout = timeout end private @@ -21,10 +23,10 @@ def client end def options - { timeout: Time.current + default_timeout } + { timeout: @timeout || client_timeout } end - def default_timeout + def client_timeout topology_service_config.timeout || DEFAULT_TIMEOUT end diff --git a/spec/lib/gitlab/topology_service_client/base_service_spec.rb b/spec/lib/gitlab/topology_service_client/base_service_spec.rb index e74a6d0fd268b2..13c92ebe8444c2 100644 --- a/spec/lib/gitlab/topology_service_client/base_service_spec.rb +++ b/spec/lib/gitlab/topology_service_client/base_service_spec.rb @@ -199,7 +199,7 @@ it 'includes timeout in client initialization' do freeze_time do - expected_deadline = Time.current + timeout + expected_deadline = timeout expect(mock_service_class).to receive(:new).with('test:50051', :this_channel_is_insecure, hash_including( @@ -214,7 +214,7 @@ it 'uses the timeout constant' do freeze_time do - expected_deadline = Time.current + timeout + expected_deadline = timeout expect(mock_service_class).to receive(:new).with(anything, anything, hash_including(timeout: expected_deadline)).and_return(mock_service) @@ -238,7 +238,7 @@ it 'returns a hash with timeout set to DEFAULT_TIMEOUT from now' do freeze_time do - expected_deadline = Time.current + Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT + expected_deadline = Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT options = base_service.send(:options) expect(options).to eq({ timeout: expected_deadline }) -- GitLab From 24ed4e9c7547ff411ada4ce79c083d65c1065b1b Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Wed, 1 Oct 2025 10:52:55 +0200 Subject: [PATCH 4/8] Use float instead of Duration --- lib/gitlab/topology_service_client/base_service.rb | 2 +- spec/lib/gitlab/topology_service_client/base_service_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/topology_service_client/base_service.rb b/lib/gitlab/topology_service_client/base_service.rb index 3e86dd36286f29..da1b9e4a02117d 100644 --- a/lib/gitlab/topology_service_client/base_service.rb +++ b/lib/gitlab/topology_service_client/base_service.rb @@ -2,7 +2,7 @@ module Gitlab module TopologyServiceClient - DEFAULT_TIMEOUT = 0.2.seconds + DEFAULT_TIMEOUT = 0.2 class BaseService def initialize(timeout: nil) diff --git a/spec/lib/gitlab/topology_service_client/base_service_spec.rb b/spec/lib/gitlab/topology_service_client/base_service_spec.rb index 13c92ebe8444c2..ec2b23d361b98a 100644 --- a/spec/lib/gitlab/topology_service_client/base_service_spec.rb +++ b/spec/lib/gitlab/topology_service_client/base_service_spec.rb @@ -174,7 +174,7 @@ describe '#client' do let(:mock_service_class) { instance_double(Class) } let(:mock_service) { instance_double(Class, new: instance_double(Object)) } - let(:timeout) { 0.5.seconds } + let(:timeout) { 0.5 } before do stub_config(cell: { -- GitLab From 961c12994e12e56c845bff195edd31af4f3a49e9 Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Thu, 2 Oct 2025 15:32:01 +0200 Subject: [PATCH 5/8] Change timeout approach and use deadline instead --- config/initializers/1_settings.rb | 1 + .../topology_service_client/base_service.rb | 27 +++- .../base_service_spec.rb | 139 +++++++++++++----- 3 files changed, 127 insertions(+), 40 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index ebe5747ac46bfd..f053bc6436e37f 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -1166,6 +1166,7 @@ Settings.cell.topology_service_client['certificate_file'] ||= nil Settings.cell.topology_service_client['private_key_file'] ||= nil Settings.cell.topology_service_client['timeout'] ||= nil +Settings.cell.topology_service_client['claim_timeout'] ||= nil Settings.cell.topology_service_client['tls'] ||= {} Settings.cell.topology_service_client['tls']['enabled'] = true if Settings.cell.topology_service_client['tls']['enabled'].nil? Settings.cell.topology_service_client['metadata'] ||= {} diff --git a/lib/gitlab/topology_service_client/base_service.rb b/lib/gitlab/topology_service_client/base_service.rb index da1b9e4a02117d..e433cc1459bb49 100644 --- a/lib/gitlab/topology_service_client/base_service.rb +++ b/lib/gitlab/topology_service_client/base_service.rb @@ -2,7 +2,8 @@ module Gitlab module TopologyServiceClient - DEFAULT_TIMEOUT = 0.2 + DEFAULT_TIMEOUT_IN_SECONDS = 1 + CLAIM_TIMEOUT_IN_SECONDS = 0.2 class BaseService def initialize(timeout: nil) @@ -17,17 +18,29 @@ def client @client ||= service_class.new( topology_service_address, service_credentials, - interceptors: [MetadataInterceptor.new], - **options + interceptors: [MetadataInterceptor.new] ) end - def options - { timeout: @timeout || client_timeout } + def with_default_timeout(&block) + with_timeout(default_timeout, &block) end - def client_timeout - topology_service_config.timeout || DEFAULT_TIMEOUT + def with_claim_timeout(&block) + with_timeout(claim_timeout, &block) + end + + def with_timeout(timeout_seconds) + deadline = Time.current.to_f + timeout_seconds + yield deadline + end + + def default_timeout + @timeout || topology_service_config.timeout || DEFAULT_TIMEOUT_IN_SECONDS + end + + def claim_timeout + @timeout || topology_service_config.claim_timeout || CLAIM_TIMEOUT_IN_SECONDS end def cell_id diff --git a/spec/lib/gitlab/topology_service_client/base_service_spec.rb b/spec/lib/gitlab/topology_service_client/base_service_spec.rb index ec2b23d361b98a..a0be5b80a875ad 100644 --- a/spec/lib/gitlab/topology_service_client/base_service_spec.rb +++ b/spec/lib/gitlab/topology_service_client/base_service_spec.rb @@ -174,15 +174,13 @@ describe '#client' do let(:mock_service_class) { instance_double(Class) } let(:mock_service) { instance_double(Class, new: instance_double(Object)) } - let(:timeout) { 0.5 } before do stub_config(cell: { enabled: true, topology_service_client: { address: 'test:50051', - tls: { enabled: false }, - timeout: timeout + tls: { enabled: false } } }) @@ -197,51 +195,126 @@ base_service.send(:client) end - it 'includes timeout in client initialization' do - freeze_time do - expected_deadline = timeout - - expect(mock_service_class).to receive(:new).with('test:50051', :this_channel_is_insecure, - hash_including( - interceptors: array_including(instance_of(Gitlab::TopologyServiceClient::MetadataInterceptor)), - timeout: expected_deadline - ) - ).and_return(mock_service) - - base_service.send(:client) - end - end - - it 'uses the timeout constant' do - freeze_time do - expected_deadline = timeout + it 'does not include timeout in client initialization' do + expect(mock_service_class).to receive(:new).with('test:50051', :this_channel_is_insecure, + hash_including( + interceptors: array_including(instance_of(Gitlab::TopologyServiceClient::MetadataInterceptor)) + ) + ).and_return(mock_service) - expect(mock_service_class).to receive(:new).with(anything, anything, - hash_including(timeout: expected_deadline)).and_return(mock_service) - - base_service.send(:client) - end + base_service.send(:client) end end - describe '#options' do + describe 'timeout methods' do before do stub_config(cell: { enabled: true, topology_service_client: { address: 'test:50051', tls: { enabled: false }, - timeout: nil + timeout: nil, + claim_timeout: nil } }) end - it 'returns a hash with timeout set to DEFAULT_TIMEOUT from now' do - freeze_time do - expected_deadline = Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT - options = base_service.send(:options) + describe '#default_timeout' do + it 'returns DEFAULT_TIMEOUT_IN_SECONDS when no config is set' do + expect(base_service.send(:default_timeout)).to eq(Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT_IN_SECONDS) + end + + context 'when timeout is configured' do + before do + stub_config(cell: { + enabled: true, + topology_service_client: { + address: 'test:50051', + tls: { enabled: false }, + timeout: 2.5 + } + }) + end + + it 'returns the configured timeout' do + expect(base_service.send(:default_timeout)).to eq(2.5) + end + end + + context 'when initialized with custom timeout' do + subject(:base_service) { described_class.new(timeout: 3.0) } + + it 'returns the custom timeout' do + expect(base_service.send(:default_timeout)).to eq(3.0) + end + end + end + + describe '#claim_timeout' do + it 'returns CLAIM_TIMEOUT_IN_SECONDS when no config is set' do + expect(base_service.send(:claim_timeout)).to eq(Gitlab::TopologyServiceClient::CLAIM_TIMEOUT_IN_SECONDS) + end + + context 'when claim_timeout is configured' do + before do + stub_config(cell: { + enabled: true, + topology_service_client: { + address: 'test:50051', + tls: { enabled: false }, + claim_timeout: 0.5 + } + }) + end + + it 'returns the configured claim timeout' do + expect(base_service.send(:claim_timeout)).to eq(0.5) + end + end + + context 'when initialized with custom timeout' do + subject(:base_service) { described_class.new(timeout: 0.3) } + + it 'returns the custom timeout' do + expect(base_service.send(:claim_timeout)).to eq(0.3) + end + end + end + + describe '#with_default_timeout' do + it 'yields a deadline based on default timeout' do + freeze_time do + expected_deadline = Time.current.to_f + Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT_IN_SECONDS - expect(options).to eq({ timeout: expected_deadline }) + base_service.send(:with_default_timeout) do |deadline| + expect(deadline).to eq(expected_deadline) + end + end + end + end + + describe '#with_claim_timeout' do + it 'yields a deadline based on claim timeout' do + freeze_time do + expected_deadline = Time.current.to_f + Gitlab::TopologyServiceClient::CLAIM_TIMEOUT_IN_SECONDS + + base_service.send(:with_claim_timeout) do |deadline| + expect(deadline).to eq(expected_deadline) + end + end + end + end + + describe '#with_timeout' do + it 'yields a deadline calculated from the given timeout' do + freeze_time do + custom_timeout = 5.0 + expected_deadline = Time.current.to_f + custom_timeout + + base_service.send(:with_timeout, custom_timeout) do |deadline| + expect(deadline).to eq(expected_deadline) + end + end end end end -- GitLab From e7257592b75094e55c65296b6e5e42302cca321e Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Fri, 3 Oct 2025 14:07:48 +0200 Subject: [PATCH 6/8] Rename claim_timeout to strict_timeout --- config/initializers/1_settings.rb | 2 +- .../topology_service_client/base_service.rb | 10 +++---- .../base_service_spec.rb | 26 +++++++++---------- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index f053bc6436e37f..e4a721f471bfb2 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -1166,7 +1166,7 @@ Settings.cell.topology_service_client['certificate_file'] ||= nil Settings.cell.topology_service_client['private_key_file'] ||= nil Settings.cell.topology_service_client['timeout'] ||= nil -Settings.cell.topology_service_client['claim_timeout'] ||= nil +Settings.cell.topology_service_client['strict_timeout'] ||= nil Settings.cell.topology_service_client['tls'] ||= {} Settings.cell.topology_service_client['tls']['enabled'] = true if Settings.cell.topology_service_client['tls']['enabled'].nil? Settings.cell.topology_service_client['metadata'] ||= {} diff --git a/lib/gitlab/topology_service_client/base_service.rb b/lib/gitlab/topology_service_client/base_service.rb index e433cc1459bb49..06988562bf88d3 100644 --- a/lib/gitlab/topology_service_client/base_service.rb +++ b/lib/gitlab/topology_service_client/base_service.rb @@ -3,7 +3,7 @@ module Gitlab module TopologyServiceClient DEFAULT_TIMEOUT_IN_SECONDS = 1 - CLAIM_TIMEOUT_IN_SECONDS = 0.2 + STRICT_TIMEOUT_IN_SECONDS = 0.2 class BaseService def initialize(timeout: nil) @@ -26,8 +26,8 @@ def with_default_timeout(&block) with_timeout(default_timeout, &block) end - def with_claim_timeout(&block) - with_timeout(claim_timeout, &block) + def with_strict_timeout(&block) + with_timeout(strict_timeout, &block) end def with_timeout(timeout_seconds) @@ -39,8 +39,8 @@ def default_timeout @timeout || topology_service_config.timeout || DEFAULT_TIMEOUT_IN_SECONDS end - def claim_timeout - @timeout || topology_service_config.claim_timeout || CLAIM_TIMEOUT_IN_SECONDS + def strict_timeout + @timeout || topology_service_config.strict_timeout || STRICT_TIMEOUT_IN_SECONDS end def cell_id diff --git a/spec/lib/gitlab/topology_service_client/base_service_spec.rb b/spec/lib/gitlab/topology_service_client/base_service_spec.rb index a0be5b80a875ad..d3e6d7c784a725 100644 --- a/spec/lib/gitlab/topology_service_client/base_service_spec.rb +++ b/spec/lib/gitlab/topology_service_client/base_service_spec.rb @@ -214,7 +214,7 @@ address: 'test:50051', tls: { enabled: false }, timeout: nil, - claim_timeout: nil + strict_timeout: nil } }) end @@ -250,25 +250,25 @@ end end - describe '#claim_timeout' do - it 'returns CLAIM_TIMEOUT_IN_SECONDS when no config is set' do - expect(base_service.send(:claim_timeout)).to eq(Gitlab::TopologyServiceClient::CLAIM_TIMEOUT_IN_SECONDS) + describe '#strict_timeout' do + it 'returns STRICT_TIMEOUT_IN_SECONDS when no config is set' do + expect(base_service.send(:strict_timeout)).to eq(Gitlab::TopologyServiceClient::STRICT_TIMEOUT_IN_SECONDS) end - context 'when claim_timeout is configured' do + context 'when strict_timeout is configured' do before do stub_config(cell: { enabled: true, topology_service_client: { address: 'test:50051', tls: { enabled: false }, - claim_timeout: 0.5 + strict_timeout: 0.5 } }) end - it 'returns the configured claim timeout' do - expect(base_service.send(:claim_timeout)).to eq(0.5) + it 'returns the configured strict timeout' do + expect(base_service.send(:strict_timeout)).to eq(0.5) end end @@ -276,7 +276,7 @@ subject(:base_service) { described_class.new(timeout: 0.3) } it 'returns the custom timeout' do - expect(base_service.send(:claim_timeout)).to eq(0.3) + expect(base_service.send(:strict_timeout)).to eq(0.3) end end end @@ -293,12 +293,12 @@ end end - describe '#with_claim_timeout' do - it 'yields a deadline based on claim timeout' do + describe '#with_strict_timeout' do + it 'yields a deadline based on strict timeout' do freeze_time do - expected_deadline = Time.current.to_f + Gitlab::TopologyServiceClient::CLAIM_TIMEOUT_IN_SECONDS + expected_deadline = Time.current.to_f + Gitlab::TopologyServiceClient::STRICT_TIMEOUT_IN_SECONDS - base_service.send(:with_claim_timeout) do |deadline| + base_service.send(:with_strict_timeout) do |deadline| expect(deadline).to eq(expected_deadline) end end -- GitLab From 92caac1128e083ae67a0d1efb7aed103bd9a3574 Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Mon, 6 Oct 2025 16:56:47 +0200 Subject: [PATCH 7/8] Rename timeout --- config/initializers/1_settings.rb | 2 +- .../topology_service_client/base_service.rb | 10 +++---- .../base_service_spec.rb | 28 ++++++++++--------- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index e4a721f471bfb2..e8c9806338a303 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -1166,7 +1166,7 @@ Settings.cell.topology_service_client['certificate_file'] ||= nil Settings.cell.topology_service_client['private_key_file'] ||= nil Settings.cell.topology_service_client['timeout'] ||= nil -Settings.cell.topology_service_client['strict_timeout'] ||= nil +Settings.cell.topology_service_client['in_transaction_timeout'] ||= nil Settings.cell.topology_service_client['tls'] ||= {} Settings.cell.topology_service_client['tls']['enabled'] = true if Settings.cell.topology_service_client['tls']['enabled'].nil? Settings.cell.topology_service_client['metadata'] ||= {} diff --git a/lib/gitlab/topology_service_client/base_service.rb b/lib/gitlab/topology_service_client/base_service.rb index 06988562bf88d3..05fa37f51f9c08 100644 --- a/lib/gitlab/topology_service_client/base_service.rb +++ b/lib/gitlab/topology_service_client/base_service.rb @@ -3,7 +3,7 @@ module Gitlab module TopologyServiceClient DEFAULT_TIMEOUT_IN_SECONDS = 1 - STRICT_TIMEOUT_IN_SECONDS = 0.2 + IN_TRANSACTION_TIMEOUT_IN_SECONDS = 0.2 class BaseService def initialize(timeout: nil) @@ -26,8 +26,8 @@ def with_default_timeout(&block) with_timeout(default_timeout, &block) end - def with_strict_timeout(&block) - with_timeout(strict_timeout, &block) + def with_in_transaction_timeout(&block) + with_timeout(in_transaction_timeout, &block) end def with_timeout(timeout_seconds) @@ -39,8 +39,8 @@ def default_timeout @timeout || topology_service_config.timeout || DEFAULT_TIMEOUT_IN_SECONDS end - def strict_timeout - @timeout || topology_service_config.strict_timeout || STRICT_TIMEOUT_IN_SECONDS + def in_transaction_timeout + @timeout || topology_service_config.in_transaction_timeout || IN_TRANSACTION_TIMEOUT_IN_SECONDS end def cell_id diff --git a/spec/lib/gitlab/topology_service_client/base_service_spec.rb b/spec/lib/gitlab/topology_service_client/base_service_spec.rb index d3e6d7c784a725..9ba6d18053d4c3 100644 --- a/spec/lib/gitlab/topology_service_client/base_service_spec.rb +++ b/spec/lib/gitlab/topology_service_client/base_service_spec.rb @@ -214,7 +214,7 @@ address: 'test:50051', tls: { enabled: false }, timeout: nil, - strict_timeout: nil + in_transaction_timeout: nil } }) end @@ -250,25 +250,27 @@ end end - describe '#strict_timeout' do - it 'returns STRICT_TIMEOUT_IN_SECONDS when no config is set' do - expect(base_service.send(:strict_timeout)).to eq(Gitlab::TopologyServiceClient::STRICT_TIMEOUT_IN_SECONDS) + describe '#in_transaction_timeout' do + it 'returns IN_TRANSACTION_TIMEOUT_IN_SECONDS when no config is set' do + expect(base_service.send(:in_transaction_timeout)).to eq( + Gitlab::TopologyServiceClient::IN_TRANSACTION_TIMEOUT_IN_SECONDS + ) end - context 'when strict_timeout is configured' do + context 'when in_transaction_timeout is configured' do before do stub_config(cell: { enabled: true, topology_service_client: { address: 'test:50051', tls: { enabled: false }, - strict_timeout: 0.5 + in_transaction_timeout: 0.5 } }) end - it 'returns the configured strict timeout' do - expect(base_service.send(:strict_timeout)).to eq(0.5) + it 'returns the configured in transaction timeout' do + expect(base_service.send(:in_transaction_timeout)).to eq(0.5) end end @@ -276,7 +278,7 @@ subject(:base_service) { described_class.new(timeout: 0.3) } it 'returns the custom timeout' do - expect(base_service.send(:strict_timeout)).to eq(0.3) + expect(base_service.send(:in_transaction_timeout)).to eq(0.3) end end end @@ -293,12 +295,12 @@ end end - describe '#with_strict_timeout' do - it 'yields a deadline based on strict timeout' do + describe '#with_in_transaction_timeout' do + it 'yields a deadline based on in transaction timeout' do freeze_time do - expected_deadline = Time.current.to_f + Gitlab::TopologyServiceClient::STRICT_TIMEOUT_IN_SECONDS + expected_deadline = Time.current.to_f + Gitlab::TopologyServiceClient::IN_TRANSACTION_TIMEOUT_IN_SECONDS - base_service.send(:with_strict_timeout) do |deadline| + base_service.send(:with_in_transaction_timeout) do |deadline| expect(deadline).to eq(expected_deadline) end end -- GitLab From 32c91e8b7a276ba646c0b6e45b1b058b594e6d23 Mon Sep 17 00:00:00 2001 From: Bojan Marjanovic Date: Tue, 7 Oct 2025 10:16:44 +0200 Subject: [PATCH 8/8] Reverts to initial approach --- config/initializers/1_settings.rb | 2 - .../topology_service_client/base_service.rb | 25 +--- .../base_service_spec.rb | 123 ++---------------- 3 files changed, 16 insertions(+), 134 deletions(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index e8c9806338a303..1c3ebd22e3f4ea 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -1165,8 +1165,6 @@ Settings.cell.topology_service_client['ca_file'] ||= nil Settings.cell.topology_service_client['certificate_file'] ||= nil Settings.cell.topology_service_client['private_key_file'] ||= nil -Settings.cell.topology_service_client['timeout'] ||= nil -Settings.cell.topology_service_client['in_transaction_timeout'] ||= nil Settings.cell.topology_service_client['tls'] ||= {} Settings.cell.topology_service_client['tls']['enabled'] = true if Settings.cell.topology_service_client['tls']['enabled'].nil? Settings.cell.topology_service_client['metadata'] ||= {} diff --git a/lib/gitlab/topology_service_client/base_service.rb b/lib/gitlab/topology_service_client/base_service.rb index 05fa37f51f9c08..1aaa4b8a0c0249 100644 --- a/lib/gitlab/topology_service_client/base_service.rb +++ b/lib/gitlab/topology_service_client/base_service.rb @@ -3,7 +3,6 @@ module Gitlab module TopologyServiceClient DEFAULT_TIMEOUT_IN_SECONDS = 1 - IN_TRANSACTION_TIMEOUT_IN_SECONDS = 0.2 class BaseService def initialize(timeout: nil) @@ -18,29 +17,13 @@ def client @client ||= service_class.new( topology_service_address, service_credentials, - interceptors: [MetadataInterceptor.new] + interceptors: [MetadataInterceptor.new], + **options ) end - def with_default_timeout(&block) - with_timeout(default_timeout, &block) - end - - def with_in_transaction_timeout(&block) - with_timeout(in_transaction_timeout, &block) - end - - def with_timeout(timeout_seconds) - deadline = Time.current.to_f + timeout_seconds - yield deadline - end - - def default_timeout - @timeout || topology_service_config.timeout || DEFAULT_TIMEOUT_IN_SECONDS - end - - def in_transaction_timeout - @timeout || topology_service_config.in_transaction_timeout || IN_TRANSACTION_TIMEOUT_IN_SECONDS + def options + { timeout: @timeout || DEFAULT_TIMEOUT_IN_SECONDS } end def cell_id diff --git a/spec/lib/gitlab/topology_service_client/base_service_spec.rb b/spec/lib/gitlab/topology_service_client/base_service_spec.rb index 9ba6d18053d4c3..61df6c4f4e5bd2 100644 --- a/spec/lib/gitlab/topology_service_client/base_service_spec.rb +++ b/spec/lib/gitlab/topology_service_client/base_service_spec.rb @@ -172,8 +172,11 @@ end describe '#client' do - let(:mock_service_class) { instance_double(Class) } + let(:mock_service_class) { class_double(Class) } let(:mock_service) { instance_double(Class, new: instance_double(Object)) } + let(:timeout) { 0.5 } + + let(:base_service) { described_class.new(timeout: timeout) } before do stub_config(cell: { @@ -195,10 +198,11 @@ base_service.send(:client) end - it 'does not include timeout in client initialization' do + it 'includes timeout in client initialization' do expect(mock_service_class).to receive(:new).with('test:50051', :this_channel_is_insecure, hash_including( - interceptors: array_including(instance_of(Gitlab::TopologyServiceClient::MetadataInterceptor)) + interceptors: array_including(instance_of(Gitlab::TopologyServiceClient::MetadataInterceptor)), + timeout: timeout ) ).and_return(mock_service) @@ -206,118 +210,15 @@ end end - describe 'timeout methods' do + describe '#options' do before do - stub_config(cell: { - enabled: true, - topology_service_client: { - address: 'test:50051', - tls: { enabled: false }, - timeout: nil, - in_transaction_timeout: nil - } - }) - end - - describe '#default_timeout' do - it 'returns DEFAULT_TIMEOUT_IN_SECONDS when no config is set' do - expect(base_service.send(:default_timeout)).to eq(Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT_IN_SECONDS) - end - - context 'when timeout is configured' do - before do - stub_config(cell: { - enabled: true, - topology_service_client: { - address: 'test:50051', - tls: { enabled: false }, - timeout: 2.5 - } - }) - end - - it 'returns the configured timeout' do - expect(base_service.send(:default_timeout)).to eq(2.5) - end - end - - context 'when initialized with custom timeout' do - subject(:base_service) { described_class.new(timeout: 3.0) } - - it 'returns the custom timeout' do - expect(base_service.send(:default_timeout)).to eq(3.0) - end - end - end - - describe '#in_transaction_timeout' do - it 'returns IN_TRANSACTION_TIMEOUT_IN_SECONDS when no config is set' do - expect(base_service.send(:in_transaction_timeout)).to eq( - Gitlab::TopologyServiceClient::IN_TRANSACTION_TIMEOUT_IN_SECONDS - ) - end - - context 'when in_transaction_timeout is configured' do - before do - stub_config(cell: { - enabled: true, - topology_service_client: { - address: 'test:50051', - tls: { enabled: false }, - in_transaction_timeout: 0.5 - } - }) - end - - it 'returns the configured in transaction timeout' do - expect(base_service.send(:in_transaction_timeout)).to eq(0.5) - end - end - - context 'when initialized with custom timeout' do - subject(:base_service) { described_class.new(timeout: 0.3) } - - it 'returns the custom timeout' do - expect(base_service.send(:in_transaction_timeout)).to eq(0.3) - end - end - end - - describe '#with_default_timeout' do - it 'yields a deadline based on default timeout' do - freeze_time do - expected_deadline = Time.current.to_f + Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT_IN_SECONDS - - base_service.send(:with_default_timeout) do |deadline| - expect(deadline).to eq(expected_deadline) - end - end - end + stub_config(cell: { enabled: true }) end - describe '#with_in_transaction_timeout' do - it 'yields a deadline based on in transaction timeout' do - freeze_time do - expected_deadline = Time.current.to_f + Gitlab::TopologyServiceClient::IN_TRANSACTION_TIMEOUT_IN_SECONDS + it 'returns a hash with timeout set to DEFAULT_TIMEOUT_IN_SECONDS from now' do + options = base_service.send(:options) - base_service.send(:with_in_transaction_timeout) do |deadline| - expect(deadline).to eq(expected_deadline) - end - end - end - end - - describe '#with_timeout' do - it 'yields a deadline calculated from the given timeout' do - freeze_time do - custom_timeout = 5.0 - expected_deadline = Time.current.to_f + custom_timeout - - base_service.send(:with_timeout, custom_timeout) do |deadline| - expect(deadline).to eq(expected_deadline) - end - end - end + expect(options).to eq({ timeout: Gitlab::TopologyServiceClient::DEFAULT_TIMEOUT_IN_SECONDS }) end end end -- GitLab