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

feat(version) Automatically determine the database version when creat… #4192

Merged
merged 1 commit into from Jul 26, 2015

Conversation

@janmeier
Copy link
Member

@janmeier janmeier commented Jul 26, 2015

…ing the first connection. Re-roll of #3842

Should be good to go, just want to get a quick review on it :)

});
});
}
}).then(function () {
Copy link
Contributor

@BridgeAR BridgeAR Jul 26, 2015

Choose a reason for hiding this comment

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

A Promise should always return something in the middle of a promise chain so I recommend to rewrite this to:

var promise;
if (self.sequelize.options.databaseVersion === 0) {
  promise = self.$connect(self.config)...;
} else {
  promise = Promise.resolve();
}
return promise.then(function () { ... });

Or:

return Promise.try(function() {
  if (self.sequelize.options.databaseVersion !== 0) {
    return;
  }
  return self.$connect(self.config)...;
}).then(...);

Copy link
Contributor

@mickhansen mickhansen Jul 26, 2015

Choose a reason for hiding this comment

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

return; would not be returning something, i don't really see the importance in any case, but up to @janmeier

Copy link
Contributor

@mickhansen mickhansen Jul 26, 2015

Choose a reason for hiding this comment

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

Although i do generally prefer short circuit code like your second example, although that example will still race.

Copy link
Contributor

@BridgeAR BridgeAR Jul 26, 2015

Choose a reason for hiding this comment

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

return; is sugar for return undefined;. So it'll return undefined and not nothing ;). I personally use return void 0; but that's just something I got used to quite a while ago. If you do not return something in the middle of a promise chain the promise thinks it's done and has to realize that there's another then chained.

@BridgeAR
Copy link
Contributor

@BridgeAR BridgeAR commented Jul 26, 2015

LGTM with one small comment

resolve(connection);
}, options.priority, options.type, options.useMaster);
return Promise.resolve().then(function () {
if (self.sequelize.options.databaseVersion === 0) {
Copy link
Contributor

@mickhansen mickhansen Jul 26, 2015

Choose a reason for hiding this comment

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

This will race, won't it?

Copy link
Member Author

@janmeier janmeier Jul 26, 2015

Choose a reason for hiding this comment

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

If several concurrent calls request a transaction yes. The other alternative is to block once the first call is sent, but it would make the code slightly more complicated. I don't really think its a big problem that call getversion maxConnections number of times :)

Copy link
Contributor

@mickhansen mickhansen Jul 26, 2015

Choose a reason for hiding this comment

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

Just if you have several concurrent calls i assume? Each will request a new connection from the pool.
Blocking would not be too tricky, save the connect + databaseversion promise in a variable, if var is set use that to resolve.

Copy link
Member Author

@janmeier janmeier Jul 26, 2015

Choose a reason for hiding this comment

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

Yep, nothing to do with transaction :)

@janmeier janmeier force-pushed the databaseVersion branch from 633b71e to e249d5e Jul 26, 2015
janmeier added a commit that referenced this issue Jul 26, 2015
feat(version) Automatically determine the database version when creat…
@janmeier janmeier merged commit 5357f6d into master Jul 26, 2015
1 check was pending
@janmeier janmeier deleted the databaseVersion branch Jul 26, 2015
@janmeier
Copy link
Member Author

@janmeier janmeier commented Jul 26, 2015

Updated, thanks for the feedback @mickhansen and @BridgeAR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants