Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds the ability to retry for the pool of connection if specified in makara config #338

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ spec/reports
test/tmp
test/version_tmp
tmp
.idea
4 changes: 2 additions & 2 deletions lib/makara/connection_wrapper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,10 @@ def _makara_connected?
false
end

def _makara_connection
def _makara_connection(force_new = false)
current = @connection

if current
if current && !force_new
current
else # blacklisted connection or initial error
new_connection = @proxy.graceful_connection_for(@config)
Expand Down
45 changes: 43 additions & 2 deletions lib/makara/proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,12 @@ def hijack_method(*method_names)

method_names.each do |method_name|
define_method method_name do |*args, &block|
appropriate_connection(method_name, args) do |con|
con.send(method_name, *args, &block)

makara_config = @config.with_indifferent_access[:makara]
if makara_config.key?(:retry_exceptions) && (retry_exceptions = makara_config[:retry_exceptions]).present?
_execute_with_connection_and_retry_exceptions(retry_exceptions, args, block, method_name)
else
_execute_with_connection(args, block, method_name)
end
end
end
Expand Down Expand Up @@ -162,6 +166,43 @@ def any_connection
end
end

MAX_RETRY_COUNT = 10

def _execute_with_connection_and_retry_exceptions(retry_exceptions, args, block, method_name)
begin
should_retry = false
potentially_stale_connection = nil

appropriate_connection(method_name, args) do |con|
con.send(method_name, *args, &block)
potentially_stale_connection = con
end

rescue Exception => actual_exception
retry_attempts = retry_exceptions.inject({}) { |memo, retry_exception| memo[retry_exception['name']] = 0; memo }
retry_exceptions.each do |retry_exception|
if actual_exception.class.to_s == retry_exception['name']
potentially_stale_connection.disconnect!
sleep retry_exception['time_between_retries_in_seconds'] || 0.1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if a bit of randomization here would be useful to avoid a load spike after the instance comes up.

retry_attempt = (retry_attempts[actual_exception.class.to_s] += 1)

max_attempts = retry_exception['retry_count'] || MAX_RETRY_COUNT

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better name would be DEFAULT_RETRY_COUNT since it's not a maximum unless you don't specify one

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a MAX count as well if you see the condition below :)

if retry_attempt < max_attempts && retry_attempt < MAX_RETRY_COUNT
should_retry = true
end
end
end
retry if should_retry
raise actual_exception
end
end

def _execute_with_connection(args, block, method_name)
appropriate_connection(method_name, args) do |con|
con.send(method_name, *args, &block)
end
end

# based on the method_name and args, provide the appropriate connection
# mark this proxy as hijacked so the underlying connection does not attempt to check
# with back with this proxy.
Expand Down