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

chore(package): update generic-pool to 3.5.0 #10359

Merged
merged 1 commit into from
Jan 18, 2019
Merged

chore(package): update generic-pool to 3.5.0 #10359

merged 1 commit into from
Jan 18, 2019

Conversation

DiegoRBaquero
Copy link

Includes some bug fixes, specifically in evicting. Meant for v4

Pull Request check-list

Please make sure to review and check all of these items:

  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Update to latest generic-pool in v4

@codecov
Copy link

codecov bot commented Jan 17, 2019

Codecov Report

Merging #10359 into v4 will not change coverage.
The diff coverage is n/a.

@sushantdhiman sushantdhiman merged commit 52daac1 into sequelize:v4 Jan 18, 2019
@DiegoRBaquero DiegoRBaquero deleted the patch-1 branch January 18, 2019 17:20
@karthikv
Copy link
Contributor

@sushantdhiman This bug in generic-pool is very significant, and can have a large effect on performance. Prior to @DiegoRBaquero's fixes in generic-pool (see coopernurse/node-pool#242), every eviction cycle would cause every idle connection to be evicted [1], regardless of the specified idle time (except those needed to meet the minimum size).

The default eviction duration is 10 seconds in v4, so every idle connection would be evicted every 10 seconds. This is further exacerbated by the default minimum pool size of 0, so this would cause constant, periodic evictions leading to many unnecessary reconnects.

Given this information, I think it'd be wise to publish a new version that contains this fix sooner than later, and perhaps mention this issue explicitly in the changelog or release history so users know. Does this sound reasonable to you?

[1] Given that softIdleTimeoutMillis is -1, which is the default value that's implicitly used by sequelize. See coopernurse/node-pool#192 for details.

@DiegoRBaquero
Copy link
Author

I'm going to add that, because of coopernurse/node-pool#243, previously, any 1 (idle) connection would be evicted every 10 seconds, but now, up to 3 really idle connections are evicted every 10 seconds. By default.

@karthikv
Copy link
Contributor

Thanks for the clarification, @DiegoRBaquero! My comment about every idle connection being evicted is incorrect due to the numTestsPerEvictionRun option, and @DiegoRBaquero's explanation above is accurate. Still, having 1 connection evicted every 10 seconds without the fixes in generic-pool 3.5.0 can cause significant performance penalties.

@sushantdhiman
Copy link
Contributor

🎉 This PR is included in version 4.42.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants