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

refactor: use sequelize-pool for pooling #10051

Merged
merged 5 commits into from Oct 28, 2018
Merged

refactor: use sequelize-pool for pooling #10051

merged 5 commits into from Oct 28, 2018

Conversation

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Oct 20, 2018

Pull Request check-list

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

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

This PR will migrate Sequelize to https://github.com/sushantdhiman/sequelize-pool which is a modernized fork of generic-pool@2.5

In theory this should solve issues like

@codecov
Copy link

@codecov codecov bot commented Oct 20, 2018

Codecov Report

Merging #10051 into master will increase coverage by 0.04%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10051      +/-   ##
==========================================
+ Coverage    96.3%   96.35%   +0.04%     
==========================================
  Files          63       63              
  Lines        9412     9399      -13     
==========================================
- Hits         9064     9056       -8     
+ Misses        348      343       -5
Impacted Files Coverage Δ
lib/sequelize.js 96.62% <ø> (ø) ⬆️
lib/dialects/mssql/connection-manager.js 88.6% <100%> (ø) ⬆️
lib/dialects/postgres/connection-manager.js 93.8% <100%> (+0.16%) ⬆️
lib/dialects/mysql/connection-manager.js 96.55% <100%> (+1.31%) ⬆️
lib/dialects/abstract/connection-manager.js 90% <87.5%> (+2.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be958ac...8c24ca8. Read the comment docs.

lib/dialects/abstract/connection-manager.js Outdated Show resolved Hide resolved
@sushantdhiman sushantdhiman removed the wip label Oct 28, 2018
@sushantdhiman sushantdhiman changed the title [WIP] refactor: use sequelize-pool for pooling refactor: use sequelize-pool for pooling Oct 28, 2018
@sushantdhiman sushantdhiman merged commit 567c019 into master Oct 28, 2018
3 of 4 checks passed
3 of 4 checks passed
@codecov
codecov/patch 91.66% of diff hit (target 96.3%)
Details
@codecov
codecov/project 96.35% (+0.04%) compared to be958ac
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sushantdhiman sushantdhiman deleted the pooling branch Oct 28, 2018
mikeringrose pushed a commit to mikeringrose/sequelize that referenced this pull request Oct 30, 2018
@mikeringrose mikeringrose mentioned this pull request Oct 31, 2018
2 of 5 tasks complete
@SystemDisc
Copy link

@SystemDisc SystemDisc commented Feb 26, 2019

Is there any way this change can be implemented in v4? We're currently experiencing occasional issues with RequestResource timed out being thrown in generic-pool when attempting to run a query like modelInstance.find(...). We believe this change would resolve the issue we're having, but we cannot use beta software.

If this change cannot be implemented in v4. Is there any estimate on when v5 might come out of beta?

Thank you for your time.

@SimonSchick
Copy link
Member

@SimonSchick SimonSchick commented Feb 26, 2019

So that's what that is, we experienced the same issue.
@sushantdhiman I think it's fair to roll out v5 at this point, it has enough changes and fixes as is.

@kelvintaywl
Copy link

@kelvintaywl kelvintaywl commented Feb 27, 2019

that would be really great! 🙇
we are just about to deliberate on using v5 beta for a fix.

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

Successfully merging this pull request may close these issues.

None yet

4 participants