Commit 0f27854a authored by Yorick Peterse's avatar Yorick Peterse

Don't retry errors when there are no replicas

In CI we don't have any replicas set up. In this case retrying any
connection errors (e.g. when a job runs without a DB set up) can
increase the job timings.

To fix this, we simply don't retry connection errors in case no replicas
are used. This also ensures the retry behaviour is the same as before
the introduction of always enabling the load balancing code.

Finally, we no longer treat NoDatabaseError errors as connection errors.
There's no point in retrying these errors, even if replicas are present,
as retrying isn't magically going to make your database exist.

See https://gitlab.com/gitlab-org/gitlab/-/issues/341757 for more
information.

Changelog: fixed
parent c1089d13
......@@ -30,6 +30,10 @@ module Gitlab
end
end
def primary_only?
@primary_only
end
def disconnect!(timeout: 120)
host_list.hosts.each { |host| host.disconnect!(timeout: timeout) }
end
......@@ -151,6 +155,17 @@ module Gitlab
# Yields a block, retrying it upon error using an exponential backoff.
def retry_with_backoff(retries = 3, time = 2)
# In CI we only use the primary, but databases may not always be
# available (or take a few seconds to become available). Retrying in
# this case can slow down CI jobs. In addition, retrying with _only_
# a primary being present isn't all that helpful.
#
# To prevent this from happening, we don't make any attempt at
# retrying unless one or more replicas are used. This matches the
# behaviour from before we enabled load balancing code even if no
# replicas were configured.
return yield if primary_only?
retried = 0
last_error = nil
......@@ -176,6 +191,11 @@ module Gitlab
def connection_error?(error)
case error
when ActiveRecord::NoDatabaseError
# Retrying this error isn't going to magically make the database
# appear. It also slows down CI jobs that are meant to create the
# database in the first place.
false
when ActiveRecord::StatementInvalid, ActionView::Template::Error
# After connecting to the DB Rails will wrap query errors using this
# class.
......
......@@ -274,6 +274,14 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do
expect { lb.retry_with_backoff { raise } }.to raise_error(RuntimeError)
end
it 'skips retries when only the primary is used' do
allow(lb).to receive(:primary_only?).and_return(true)
expect(lb).not_to receive(:sleep)
expect { lb.retry_with_backoff { raise } }.to raise_error(RuntimeError)
end
end
describe '#connection_error?' do
......@@ -283,6 +291,12 @@ RSpec.describe Gitlab::Database::LoadBalancing::LoadBalancer, :request_store do
expect(lb.connection_error?(error)).to eq(true)
end
it 'returns false for a missing database error' do
error = ActiveRecord::NoDatabaseError.new
expect(lb.connection_error?(error)).to eq(false)
end
it 'returns true for a wrapped connection error' do
wrapped = wrapped_exception(ActiveRecord::StatementInvalid, ActiveRecord::ConnectionNotEstablished)
......
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