Introduce parameters to control the connection pool for engine creation #14
Conversation
|
It might be interesting to set these via ENV, so that you can set them for each container individually without having to add the to the respective config schema and pass them down to the db_engine. |
qwc_services_core/database.py
Outdated
| self.engines = {} | ||
|
|
||
| def db_engine(self, conn_str): | ||
| def db_engine(self, conn_str, pool_size=10, max_overflow=20, pool_timeout=30, pool_recycle=300): |
There was a problem hiding this comment.
Are these the defaults that SQLAlchemy already has? Because with my QGIS Cloud hat on both pool_size and max_overflow seem to be quite high. I'd actually expect them to cause troubles with QGIS Cloud...
https://docs.sqlalchemy.org/en/20/core/engines.html#engine-creation-api states that default pool_size is 5. Do you have a good reason to double it to 10? Same with max_overflow.
pool_recycle default is -1 i.e. no recycling of connections. What's the reason to set it to 300?
My concern doesn't maybe carry much weight because possibly this wouldn't have any impact on QGIS Cloud anyway. @mwa ?
There was a problem hiding this comment.
Shure, If there are official default values, they should be used! Didn't do my homework to find them and just set values, that there is something. Good idea, to set them via ENV, I will have a look at that aswell.
| db_max_overflow = self.db_engine_env('MAX_OVERFLOW', max_overflow) | ||
| db_pool_timeout = self.db_engine_env('POOL_TIMEOUT', pool_timeout) | ||
| db_pool_recycle = self.db_engine_env('POOL_RECYCLE', pool_recycle) | ||
|
|
There was a problem hiding this comment.
I'd set the default values directly here and remove them from the db_engine args.
Also, self.db_engine_var does not return an environment variable value, but invokes self.db_engine to return a DB Engine configured with the connections string as configured in the specified environment variable. Use os.environ.get here.
| 'db_engine_env: Environment variable %s not set' % env_name) | ||
| return self.db_engine(conn_str) | ||
| return self.db_engine(env_value) | ||
|
|
There was a problem hiding this comment.
These changes here should be reverted
|
Thank you for adding this little PR to my credits, as you could have done this by yourself in 2 minutes ... |
|
Perhaps
but no need really to be harsh. |
3 participants
Consider qwc-services/qwc-config-generator#88 (comment) to make the connection pool configurable.