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

Make DB connection options more transparent #2030

Closed
maturanomx opened this issue Sep 26, 2018 · 6 comments
Closed

Make DB connection options more transparent #2030

maturanomx opened this issue Sep 26, 2018 · 6 comments
Labels
issue: enhancement Issue suggesting an enhancement to an existing feature severity: medium If it breaks the basic use of the product but can be worked around

Comments

@maturanomx
Copy link
Contributor

maturanomx commented Sep 26, 2018

What is the expected behavior?
Strapi uses Knexjs for manage DB connections, but it's not using all the options available.


This story begins with me trying to deploy Strapi on Pivotal's Could Foundry using the free MySQL service plan with a limitation up to 4 connections:
capture

Apparently, Strapi is using more that than just for starting according the logs:

[APP/PROC/WEB/0] ERR (node:147) UnhandledPromiseRejectionWarning: Error: ER_USER_LIMIT_REACHED: User 'bf662ec45746cc' has exceeded the 'max_user_connections' resource (current value: 4)

Looking for a solution, I see Knexjs allows to configure the pool connection:

To change the config settings for the pool, pass a pool option as one of the keys in the initialize block

But seeing the code:

  1. Strapi pick just some specific options
  2. Some options are limited by engine (like this, the pool one]

Question: Wouldn't it be better to make this configuration match Knexjs? The Strapi database configuration should match Knexjs available options.

Request for comments before begin to work with a patch.

@derrickmehaffy
Copy link
Member

+1

@lauriejim lauriejim added issue: enhancement Issue suggesting an enhancement to an existing feature severity: medium If it breaks the basic use of the product but can be worked around labels Sep 27, 2018
@Aurelsicoko
Copy link
Member

@maturanomx Good catch! We could remove the limitation, your use case justifies the changes. Can you make a PR for this?

@maturanomx
Copy link
Contributor Author

Sure! 👍

@Aurelsicoko
Copy link
Member

Awesome!!

@lauriejim lauriejim added this to 🕰Waiting for classification in 🥘Cooking via automation Jan 17, 2019
@lauriejim lauriejim moved this from 🕰Waiting for classification to 🤨To check in 🥘Cooking Jan 17, 2019
@lauriejim lauriejim added the good first issue Good for newcomers label Feb 9, 2019
@derrickmehaffy derrickmehaffy added this to Needs triage in Good for new contributors Mar 26, 2019
@derrickmehaffy derrickmehaffy moved this from Needs triage to Low priority in Good for new contributors Mar 26, 2019
@derrickmehaffy derrickmehaffy moved this from Low priority to Medium priority in Good for new contributors Mar 26, 2019
@lauriejim lauriejim added note: to discuss and removed good first issue Good for newcomers labels May 9, 2019
@petersg83
Copy link
Contributor

Is this issue still ongoing?

@derrickmehaffy
Copy link
Member

Is this issue still ongoing?

I don't believe it's an issue anymore as we now pass along the tarn.js option parameters through to knex (connection pooling options). Going to mark this as closed, please let us know if we missed something and we can reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: enhancement Issue suggesting an enhancement to an existing feature severity: medium If it breaks the basic use of the product but can be worked around
Projects
No open projects
🥘Cooking
🤨To check
Development

No branches or pull requests

5 participants