-
Notifications
You must be signed in to change notification settings - Fork 822
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
NEW Allow database read-only replicas #11379
NEW Allow database read-only replicas #11379
Conversation
4554d2e
to
a734683
Compare
4015f1a
to
4e739a4
Compare
src/Control/Director.php
Outdated
* List of rules that must only use the primary database and not a replica | ||
*/ | ||
private static array $must_use_primary_db_rules = [ | ||
'Security' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm enforcing must_use_primary on anything "Security" related - i.e. any url that matches /Security, any DataObject under the Security namespace, to prevent any weird issues that could happen when a replica is out of sync with the primary
For instance a password is changed, the user logs out, but cannot log back in because the replica isn't synced up yet.
Having replicas be slightly out of sync is one of the trade-offs for using replicas, though for anything security related it introduces risk so it's disallowed
bc7692c
to
2d388e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested locally yet but here's some requested changes based on reviewing the code
e57f963
to
2d41361
Compare
75f7c5c
to
d66c230
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- NEW Allow database read-only replicas #11379 (comment) still hasn't been addressed
- New comment in NEW Allow database read-only replicas #11379 (comment) (sorry, forgot to start review)
af8c2ac
to
2dbb1a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a couple of changes which make it seem like this PR probably wasn't updated fully when you retargetted 6? Please double check before I continue my review.
0a6f3db
to
4aaca65
Compare
There's also some failing CI - does some of that rely on other PRs? |
I think the unit test failures are related to the branch having /5/ in it when it's a CMS 6 PR (originally it pointed to CMS 6) - our CI looks at the branch pattern to get the version of installer Issue links to a recipe sink build so we can rely on that |
4aaca65
to
f5ef850
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue #11352