geopython / GeoHealthCheck

Service Status and QoS Checker for OGC Web Services
https://geohealthcheck.org
MIT License
84 stars 71 forks source link

adding pre_ping to sqlalchemy create_engine option #400

Closed epifanio closed 3 years ago

epifanio commented 3 years ago

This option should mitigate a problem arising when running GeoHealthCheck in a docker swarm environment with a PostgreSQL database backend.

epifanio commented 3 years ago

will work something like:

Adding:

SQLALCHEMY_ENGINE_OPTION_PRE_PING = False

in config_main.py

and then change init.py to:

        SQLALCHEMY_ENGINE_OPTIONS = {
                                 'pool_pre_ping': app.config['SQLALCHEMY_ENGINE_OPTION_PRE_PING']
                                 }
        App.db_instance = SQLAlchemy(app, 
                                     engine_options=SQLALCHEMY_ENGINE_OPTIONS)
fsteggink commented 3 years ago

Although this fix will work in the Swarm case, it will add a small overhead to each and every DB-call as 'under the hood' SQLAlchemy will issue a SELECT 1 before the actual DB call. So I would rather see this as a configurable option, with a default of False. At this moment it is hard to assess the overhead. Anyone has figures? @tomkralidis or @fsteggink ? If the overhead is like under 1% we may hardcode it to True. Still I would also like the SQLALCHEMY_ENGINE_OPTIONS structure in config_main.py. If we go for a parameter we could call it SQLALCHEMY_ENGINE_OPTION_PRE_PING, set it during config initialization. Then it can also be documented.

I'd like to have this as a configurable option. GHC can already be quite heavy on the DB (as I've encountered), so if extra overhead can be avoided, that would be definitely my preference.

epifanio commented 3 years ago

I'd like to have this as a configurable option. GHC can already be quite heavy on the DB (as I've encountered), so if extra overhead can be avoided, that would be definitely my preference.

The revised PR implements the SQLALCHEMY_ENGINE_OPTION_PRE_PING as configurable options, by default set to FALSE.

justb4 commented 3 years ago

Sorry there is still a slight coding convention error:

Run flake8
./GeoHealthCheck/init.py:87:80: E501 line too long (97 > 79 characters)
./GeoHealthCheck/init.py:89:42: W291 trailing whitespace

Please always run flake8 before committing. pip install flake8 and run flake8 at root of project.

justb4 commented 3 years ago

Don't mind the failing tests, has to do with remote endpoints.