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

Conversation

vishnun
Copy link

@vishnun vishnun commented Nov 13, 2021

This is an attempt to do graceful failover by re-trying at the lowest level.
(This isn't ready to be merged in master for makara but we'll use this branch)

Example config:

development:
  database: speckel_development
  adapter: postgresql_makara
  host: localhost
  username: speckel
  password: 23432
  pool: 20
  makara:
    id: 'development-app-postgres'
    disable_blacklist: true
    master_ttl: 0.3
    sticky: true
    retry_exceptions:
      - name: ActiveRecord::StatementInvalid
        retry_count: 5
        time_between_retries_in_seconds: 0.2
    connections:
      - role: master
        name: DB-Writer
        host: localhost
        port: 5432
        blacklist_duration: 0
      - role: slave
        name: DB-Reader
        host: localhost
        port: 5433

iain-bryson
iain-bryson previously approved these changes Nov 13, 2021
Copy link

@iain-bryson iain-bryson left a comment

Choose a reason for hiding this comment

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

LGTM some comments that are completely optional and I don't think are obviously worth resetting testing.

sleep retry_exception['time_between_retries_in_seconds'] || 0.1
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 :)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants