Skip to content

PoolСluster auto-restore node #906

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

PoolСluster auto-restore node #906

wants to merge 5 commits into from

Conversation

eakorolev
Copy link
Contributor

dougwilson
dougwilson reviewed Sep 9, 2014
View reviewed changes
lib/PoolCluster.js
node.online = false;
this.emit('offline', node.id);

setTimeout(function() {
Copy link
Member

Choose a reason for hiding this comment

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

Timeout will hold the event loop open and there is no way for the user to cancel this timeout. Please fix this so a user can close their pools and allow the event loop to stop.

@eakorolev
Copy link
Contributor Author

@dougwilson, can you please check this PR again? I'd like to use this feature in production.

@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 122fc22 to dc0d600 Compare September 16, 2014 16:17
@dougwilson
Copy link
Member

Sure, no problem. Sorry, GitHub does not like email or notify me when the PR is updated by a new commit (damn you GitHub, lol).

@dougwilson dougwilson self-assigned this Sep 17, 2014
@dougwilson
Copy link
Member

I don't want you to think I'm ignoring you here :) I'm on vacation right now, so I'm just working through this a little slower than normal is all.

@eakorolev
Copy link
Contributor Author

Don't worry, everything's fine :) Thank you for contributing such useful module! It's great!

@dougwilson dougwilson added this to the 2.6 milestone Sep 17, 2014
@eakorolev
Copy link
Contributor Author

Hi! @dougwilson, do you have any plans about 2.6 release?

@dougwilson
Copy link
Member

Yes! I plan to get it out the door sometime this weekend, including this PR in it, of course, unless something crazy happens.

@denchistyakov
Copy link

It will be great 👍

@eakorolev
Copy link
Contributor Author

@dougwilson, should I wait for this fix?

@jonathanrdelgado
Copy link

+1

@dougwilson
Copy link
Member

Sorry for the delay :( This PR is marked for the 2.6 milestone and I'm currently working through merging them all.

dougwilson
dougwilson reviewed Mar 9, 2015
View reviewed changes
.gitignore
@@ -8,5 +8,6 @@ npm-debug.log
/test/config.js

*.sublime-*
.idea
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this; really, editor-specific entries belong in your global ignore list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove this line, and sublime-specific things too 😃 OK?

Copy link
Member

Choose a reason for hiding this comment

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

I just went ahead and cleaned up the file on master :)

@dougwilson
Copy link
Member

Please test the version that has landed on master as thoroughly as you need so we can catch issues before being released :)

dougwilson added a commit that referenced this pull request Mar 25, 2015
@dougwilson
Add restoreNodeTimeout option to PoolCluster
8dad7a1 
closes #880
closes #906
@eakorolev
Copy link
Contributor Author

I've tested pool cluster by shutting down mysql server nodes (I had 3 of them). Works well, but sometimes pool cluster emits a lot of offline events at a time. It's not a big problem, because it is not an exception.

@eakorolev eakorolev deleted the poolcluster-autorestore branch April 1, 2015 11:25
@dougwilson
Copy link
Member

Works well, but sometimes pool cluster emits a lot of offline events at a time

I'll take a look into this.

@eakorolev
Copy link
Contributor Author

It happens when cluster is trying to restore node, but remote server still offline.

@dougwilson
Copy link
Member

Gotcha. Yes, I was able to determine the same. It's a limitation unless the PoolCluster is seriously refactored such that we make the lookup for a node to be async instead of sync.

seangarner pushed a commit to seangarner/node-mysql that referenced this pull request May 11, 2015
@dougwilson @seangarner
Add restoreNodeTimeout option to PoolCluster
e9e4cb6 
closes mysqljs#880
closes mysqljs#906
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dougwilson dougwilson

Labels
feature
Milestone
2.6
Development

Successfully merging this pull request may close these issues.
4 participants
@eakorolev @dougwilson @denchistyakov @jonathanrdelgado