Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Added option.maxTries #184

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

Open
Aleksandras-Novikovas wants to merge 4 commits into coopernurse:master
base: master
Choose a base branch
Loading
from Aleksandras-Novikovas:master

Conversation

@Aleksandras-Novikovas
Copy link

@Aleksandras-Novikovas Aleksandras-Novikovas commented Mar 10, 2017

Setting this option stops trying to create resource after specified
consequent failures. Prevents pool going into infinite loop if resource
can not be created.

joakimbeng, mrtbld, mikuso, sokoloveg, ernestrc, vladimir-golovchenko, elhigu, DeadCat, sywka, bforbis, and 4 more reacted with thumbs up emoji
Copy link

elhigu commented Nov 16, 2017

PR seems to be missing tests.

I would really need this feature for knex since now if create fails due to bad DB setup, pool just waits until acquireConnectionTimeout.

This feature would allow us to decide if we want to wait only in that case if create() method itself is waiting or pool is full. We would get an error directly in case if create() call rejected. For now it looks like I need to implement timeout back to knex side or do some other nasty hacks.

@coopernurse If this PR is fixed is will it be merged?

Setting this option stops trying to create resource after specified
consequent failures. Prevents pool going into infinite loop if resource
can not be created.
Copy link
Author

I have re-based my pull request against latest version.

@sandfox sandfox self-assigned this Feb 12, 2018
Copy link
Author

I've added tests for maxTries feature.
Could you merge this PR?
I really need it. As I can see many others need this functionality too.

gsbelarus reacted with thumbs up emoji

Copy link

Is it hard to merge this PR? We desperately need in ability to stop attempts to create resource (attach to database in our case) after first try failed.

Copy link
Collaborator

sandfox commented Jun 27, 2019

Is it possible to use the existing

pool.on('factoryCreateError', function(err){
 //log stuff maybe
})

to achieve the same outcome, or is that sufficiently overcomplicated to pipe together?

Copy link

Consider we are pooling connections to db server. Somehow wrong login/password were specified. Current behavior is that the library will try to reconnect again and again... Correct work in this case would be only one attempt and then throw exception to a calling code.

Now, to emulate this we do first connection without pooling. If it succeeds then we disconnect and run through pool library. If it fails we analyze return value and throw appropriate exception.

Copy link

Callback factoryCreateErrorwould be useful if we could:

  1. know exactly which acquire call fails
  2. stop further attempts
  3. bubble up exception to a calling code

Copy link

elhigu commented Jun 27, 2019

@gsbelarus is it your own code which is using node-pool or 3rd party library?

Copy link

gsbelarus commented Jun 27, 2019 via email

our own.
...
On Thu, Jun 27, 2019 at 5:49 PM Mikael Lepistö ***@***.***> wrote: @gsbelarus <https://github.com/gsbelarus> is it your own code which is using node-pool or 3rd party library? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#184?email_source=notifications&email_token=ABHPTVG3XM5LBYZTNPAY6BLP4THODA5CNFSM4DDGGNGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYXLRDY#issuecomment-506378383>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABHPTVEOI4HCLFS7ASY3UXLP4THODANCNFSM4DDGGNGA> .

Copy link

elhigu commented Jun 27, 2019

We ended up writing our own pool implementation which hat that error propagation feature in addition to various timeout configurations to be used with knex. You migth want to take a look https://github.com/vincit/tarn.js propagateCreateError is the option for that.

That pool was written for knex, so it will probably not get any additional features in the future, but those current features are really robust.

gsbelarus and oxc reacted with thumbs up emoji

Copy link

Thanks! Will give it a try.

Copy link
Collaborator

Please resolve the conflicts and I will be happy to merge this!

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

Reviewers

No reviews

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /