-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
enh: Implement PrimaryReadReplicaConnection #41998
Conversation
Is this ready for testing, or does it need more work you think? |
65e3016
to
2d2ef65
Compare
65089ee
to
55cfca8
Compare
6882810
to
1a1e6f9
Compare
Signed-off-by: Julius Härtl <jus@bitgrid.net>
1a1e6f9
to
79c4986
Compare
Should be good now for testing @mickenordin :) |
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 can see how wrong development can nullify the efforts, but it's a start and can only get better forward
Is this only for performance, or will there be failover to a replica, if the primary fails? |
Only for performance and read only replicas. |
Just to mention, there is some follow up for that already in the queue at #42345 |
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.
Makes sense!!
@@ -237,7 +233,11 @@ public function createConnectionParams(string $configPrefix = '') { | |||
$connectionParams['persistent'] = true; | |||
} | |||
|
|||
return $connectionParams; | |||
$replica = $this->config->getValue('dbreplica', []) ?: [$connectionParams]; |
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 found that one a bit strange.
Isn't that the same as
$this->config->getValue('dbreplica', [$connectionParams]);
?
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.
Cool 👍
Also without configuring dbreplica
it still works 🙌
This is really fantastic! I was really thinking about whether I should set up YugabyteDB or not! |
|
@juliushaertl Can you set this as the milestone for 28? It would be great to include this in the next minor release! |
We only backport bugfixes and this is a larger enhancement so currently only targeting Nextcloud 29. |
That's a sad thing... I'm not familiar with the release cycle of Nextcloud, so when will 29 be released? |
Current planned schedule is in https://github.com/nextcloud/server/wiki/Maintenance-and-Release-Schedule |
First approach for #33542 split to only have the read replica support in this PR. Follow up prepared in #42345
Fixes #40334
Summary
Make use of the PrimaryReadReplicaConnection class that Doctrine offers to be able to configure read replicas for Nextcloud databases.
Library docs: https://github.com/doctrine/dbal/blob/3.7.x/src/Connections/PrimaryReadReplicaConnection.php#L28C1-L76
Checklist