Skip to content

Use ActiveRecord configuration to read from replica for reporting#5272

Merged
zachmargolis merged 11 commits intomainfrom
margolis-read-replica-connections
Aug 11, 2021
Merged

Use ActiveRecord configuration to read from replica for reporting#5272
zachmargolis merged 11 commits intomainfrom
margolis-read-replica-connections

Conversation

@zachmargolis
Copy link
Copy Markdown
Contributor

Seems to work locally, going to test in my personal env shortly

Comment thread config/database.yml
pool: <%= IdentityConfig.store.database_pool_idp %>
sslmode: 'verify-full'
sslrootcert: '/usr/local/share/aws/rds-combined-ca-bundle.pem'
replica: true
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

turns out when a connection is set to replica: true Rails will block writes also, which is nice

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh sweet that's awesome

@zachmargolis
Copy link
Copy Markdown
Contributor Author

Update: seems to work mostly as expected in my personal env

Comment thread config/database.yml
Comment on lines +44 to +48
pool = if Identity::Hostdata.instance_role == 'worker'
IdentityConfig.store.good_job_max_threads * IdentityConfig.store.database_pool_idp
else
IdentityConfig.store.database_pool_idp
end
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

cc @mitchellhenke @jgrevich this was how we wanted to scale threads for workers? or did we want max_threads + pool_idp (pool is 5 here)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Jobs will most likely using at least one connection each, so adding together makes sense to me to give a little bit of breathing room. I'm not sure what the max total connections is though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to add in 216b3e9

Copy link
Copy Markdown
Contributor

@orenyk orenyk left a comment

Choose a reason for hiding this comment

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

Looks great, love all the deletions 😄

Comment thread config/database.yml Outdated
<<: *defaults
primary:
<<: *defaults
primary_replica:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

question: should we call it read_replica instead? That threw me off a little bit when you call establish_connection.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great idea, and that matches the name in devops repo where we call it read-replica, updated in 27926b2

Comment thread config/database.yml
pool: <%= IdentityConfig.store.database_pool_idp %>
sslmode: 'verify-full'
sslrootcert: '/usr/local/share/aws/rds-combined-ca-bundle.pem'
replica: true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh sweet that's awesome

@zachmargolis zachmargolis merged commit 2beb3bd into main Aug 11, 2021
@zachmargolis zachmargolis deleted the margolis-read-replica-connections branch August 11, 2021 18:02
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.

3 participants