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.

Sign up for GitHub

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

Jump to bottom

[GOVCMSD8-556]: Add read replica access. #83

Merged
merged 5 commits into from
May 5, 2020
Merged

[GOVCMSD8-556]: Add read replica access. #83

merged 5 commits into from
May 5, 2020

Conversation

steveworley
Copy link
Contributor

@steveworley steveworley commented Sep 2, 2019 •
edited
Loading

  • Updates govcms-deploy to use the read replica for
  • Adds database config for the read replica
  • Introduces MARIADB_HOST_READREPLICA environment variable to control the mysql host.

amazeeio/dbaas-mariadb-apb#5 (comment)

@simesy simesy mentioned this pull request Sep 10, 2019
Merged
simesy
simesy approved these changes Sep 10, 2019
View reviewed changes
Copy link
Contributor

@simesy simesy left a comment

Choose a reason for hiding this comment

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

I can test if a get read replica on one of my projects. Looks pretty sane to me.

@steveworley
Copy link
Contributor Author

Updated the PR to include the replica configuration for the default database service, this allows us to use the database.replica service in custom code to send requests directly to the reader where necessary.

Drupal will round robin all configured replicas when it is making this connection.

stooit
stooit approved these changes Nov 27, 2019
View reviewed changes
Copy link
Collaborator

@stooit stooit left a comment

Choose a reason for hiding this comment

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

Approved pending common_deploy suggestion

stooit
stooit requested changes Nov 27, 2019
View reviewed changes
stooit
stooit requested changes Nov 27, 2019
View reviewed changes
Copy link
Collaborator

@stooit stooit left a comment

Choose a reason for hiding this comment

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

  • Bail early if valid env var not present
  • Redirect sterr
  • Consider some output so we know which db is being used for sync
  • Unsure why drush alias changed?
  • Typos in read replica

@AlexSkrypnyk AlexSkrypnyk added the Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request label Jan 17, 2020
@AlexSkrypnyk
Copy link
Contributor

@SRowlands could you please re-review this one. thanks

@steveworley
# This is a combination of 3 commits.
2e51a3c 
# This is the 1st commit message:

Add read replica access.

Updates govcms-deploy to use the read replica for
Adds database config for the read replica
Introduces MARIADB_HOST_READREPLICA environment variable to control the mysql host.

# This is the commit message #2:

Add replicas to the default connection.
# This is the commit message #3:

Update .docker/images/govcms8/scripts/govcms-deploy
@steveworley
Add read replica access.
710663c 
Updates govcms-deploy to use the read replica for
Adds database config for the read replica
Introduces MARIADB_HOST_READREPLICA environment variable to control the mysql host.

Add replicas to the default connection.

Update .docker/images/govcms8/scripts/govcms-deploy

Code review updates.

- Return early if host is unavailable.
- Updated db_conf settings for reusability.
- Redirect show tables to stderr.
- Reverted alias change.
- Added output when script is using the reader.

Update the hosts usage.

- Lagoon has updated to use a comma separated list of host names.

Add a condition to make sure trim didn't remove empty.

Update govcms-deploy to check HOSTS.
@steveworley steveworley added Needs review Pull requests needs a review from assigned developers and removed Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request labels Jan 20, 2020
AlexSkrypnyk
AlexSkrypnyk reviewed Jan 20, 2020
View reviewed changes
Copy link
Contributor

@AlexSkrypnyk AlexSkrypnyk left a comment

Choose a reason for hiding this comment

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

@steveworley shouldn't the variable name be MARIADB_READ_REPLICA_HOSTS?

@steveworley
Copy link
Contributor Author

@AlexSkrypnyk as far as I know the Lagoon broker will be exposing it as MARIADB_READREPLICA_HOSTS https://github.com/amazeeio/dbaas-mariadb-apb/pull/5/files#diff-e349823b467059e175ddb237223092ecR50

@AlexSkrypnyk
Copy link
Contributor

I have cross-checked requested changes by @SRowlands with the latest changes in code - all points were addressed. This is ready to be merged.

@AlexSkrypnyk AlexSkrypnyk requested a review from fubarhouse April 27, 2020 00:41
@AlexSkrypnyk
Copy link
Contributor

Similar PR is coming for scaffold-tooling.

AlexSkrypnyk
AlexSkrypnyk suggested changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@AlexSkrypnyk AlexSkrypnyk left a comment

Choose a reason for hiding this comment

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

Found several issues while porting to scaffold-tooling. Will raise a PR against this PR with fixes. Please do not merge this PR yet.

@AlexSkrypnyk AlexSkrypnyk added the DO NOT MERGE Do not merge this pull request label Apr 27, 2020
@AlexSkrypnyk AlexSkrypnyk mentioned this pull request Apr 27, 2020
Merged
@AlexSkrypnyk
Copy link
Contributor

@steveworley this is ready to be merged. please merge at your earliest convenience

@AlexSkrypnyk AlexSkrypnyk added Ready to be merged Pull request is ready to be merged (assigned after testing is complete) and removed DO NOT MERGE Do not merge this pull request Needs review Pull requests needs a review from assigned developers labels Apr 28, 2020
@steveworley steveworley changed the title Add read replica access. [GOVCMSD8-556]: Add read replica access. May 5, 2020
@steveworley steveworley merged commit 40bf3fe into develop May 5, 2020
@steveworley steveworley deleted the feature/read-replica branch May 5, 2020 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexSkrypnyk AlexSkrypnyk AlexSkrypnyk requested changes

@simesy simesy simesy approved these changes

@stooit stooit Awaiting requested review from stooit

@fubarhouse fubarhouse Awaiting requested review from fubarhouse

Assignees

@steveworley steveworley

Labels
Ready to be merged Pull request is ready to be merged (assigned after testing is complete)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@steveworley @AlexSkrypnyk @simesy @stooit