-
Notifications
You must be signed in to change notification settings - Fork 159
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
base: master
Are you sure you want to change the base?
Conversation
var sequel = new Sequel(schema, sqlOptions); | ||
|
||
// Build a query for the specific query strategy | ||
try { |
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.
this seems odd, I may have squished someone elses commit here.
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.
Try pulling in the latest from master
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.
Bump? :)
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? |
I wouldn't call this 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) { |
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.
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).
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.
Looked like an internal call, but someone else may be more aware of the ramifications.
@RWOverdijk this naming convention was taken from 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 |
Ah okay that makes sense. Still though, replication...? :p |
I mean it makes sense to me from a DBA perspective because thats how the servers are actually configured for replication. |
Exciting stuff! |
@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: Not being a DBA how does this work with transactions? Does it start a transaction on both the read connection and the write connection? |
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. |
Tests still need to be made, but heres a basic proof of concept of how we would handle multiple connection pools.