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

added replication support #267

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

icodeforlove
Copy link

Tests still need to be made, but heres a basic proof of concept of how we would handle multiple connection pools.

var sequel = new Sequel(schema, sqlOptions);

// Build a query for the specific query strategy
try {
Copy link
Author

Choose a reason for hiding this comment

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

this seems odd, I may have squished someone elses commit here.

Choose a reason for hiding this comment

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

Try pulling in the latest from master

Choose a reason for hiding this comment

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

Bump? :)

@RWOverdijk
Copy link

Does this change the connection name dynamically, too? I mean, the logs are useful if they also log which server from the pools you're connecting to?

@RWOverdijk
Copy link

I wouldn't call this replication. I'm thinking balanced?

I like the initiative on this PR, 👍 for that.

@@ -1117,8 +1106,9 @@ module.exports = (function() {
* @param {Function} fn
* @param {[type]} cb
*/
function spawnConnection(connectionName, fn, cb) {
function spawnConnection(queryType, connectionName, fn, cb) {

Choose a reason for hiding this comment

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

Should be careful that this doesn't break anywhere. Did you check for all uses? Does this get exposed to user code? (if so, it's a BC break).

Copy link
Author

Choose a reason for hiding this comment

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

Looked like an internal call, but someone else may be more aware of the ramifications.

@icodeforlove
Copy link
Author

@RWOverdijk this naming convention was taken from sequelize

http://docs.sequelizejs.com/en/1.7.0/docs/usage/#read-replication

as for logs yes it logs which index and which pool its from

@RWOverdijk
Copy link

Ah okay that makes sense. Still though, replication...? :p

@icodeforlove
Copy link
Author

I mean it makes sense to me from a DBA perspective because thats how the servers are actually configured for replication.

@devinivy
Copy link
Contributor

Exciting stuff!

@particlebanana
Copy link
Contributor

@icodeforlove @RWOverdijk @devinivy this is interesting. From a glance this actually looks like something we can support on pretty much all adapters by creating multiple connection pools. Although in the sequelize docs it says: Note: Read replication only works for MySQL at the moment! is this a limitation of the actual databases?

Not being a DBA how does this work with transactions? Does it start a transaction on both the read connection and the write connection?

@devinivy
Copy link
Contributor

Read replication is a feature of certain types of databases. The databases coordinate with each other to keep multiple read replicas in sync with some master database. Transactions should be supported– I believe they occur on the master database, then the replicas lag slightly behind, catching-up on all the transactions that have occurred on master.

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

Successfully merging this pull request may close these issues.

4 participants