fix(models.core:Database): Allocate a SQLA engine once per process per URL.

[pedro-r-marques]

SUMMARY

As per issue #27897, SqlAlchemy recommends that

"""
The typical usage of create_engine() is once per particular database URL, held globally for the lifetime of a single application process. A single Engine manages many individual DBAPI connections on behalf of the process and is intended to be called upon in a concurrent fashion. The Engine is not synonymous to the DBAPI connect() function, which represents just one connection resource - the Engine is most efficient when created just once at the module level of an application, not per-object or per-function call.
"""

The superset.models.core::Database class is currently allocating Engine objects whenever a Database object is instantiated which happens twice(?) per 'api/v1/chart/data' API access. This is not the intended usage of the SQL Alchemy API and causes mechanisms such as connection pooling not to work correctly.

Connection pooling is an important feature in order to control access to databases. For instance when using 'duckdb', it is recommended that one uses a small number of concurrent requests (or a single concurrent request) and instead take advantage of the inherent parallelism in the database engine. Other engines will have other ideal settings.

TESTING INSTRUCTIONS

Configure a connection pool using the DB_CONNECTION_MUTATOR hook.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[github-actions]

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

[betodealmeida]

This is great, @pedro-r-marques, thanks for the PR!

There's been some discussion in the past regarding improving the engines and connection pooling (#8574), and your PR seems to be the first step in that direction.

[betodealmeida]

Can we have params be part of the key as well, in some deterministic serialized form? A few databases that support user impersonation will add the user info to params, and in that case the engine manager would not be very useful.

[pedro-r-marques]

PTAL

[pedro-r-marques]

The CI failures above:

• Translations was a CI error (network failure).
• pre-commit check wants to reformat lines of code in a file that I touched but have nothing to do with the PR. What is the policy for the project ? should I add the change to make 'black' happy ? Or ignore it since it is unrelated to the intended change ?

[eschutho]

@pedro-r-marques I restarted CI and fixed the formatting issue. We'll see if it passes now.

[pedro-r-marques]

@eschutho The formatting issue is fixed. Unfortunatly the test-mysql test seems to be failing in test_chart_data_async. I've attempted to run the test multiple times in my setup (using the same mysql version and redis cache configuration as the CI) with no luck.

Any hints ?

From the failure code, I'm under the impression that the test is expecting result code 202 [ which would mean that the job has been submitted to a cellery worker ? ] and is instead getting a 200 [ meaning the result is being served from cache ? ]

I've tried running the test both at the version of the code in the PR as well as at HEAD. It always passes for me.

[ShubhamDalmia]

Any updates on this?

[villebro]

From the failure code, I'm under the impression that the test is expecting result code 202 [ which would mean that the job has been submitted to a cellery worker ? ] and is instead getting a 200 [ meaning the result is being served from cache ? ]

@pedro-r-marques the particular test is hopelessly flaky (we will likely need to remove it, as fixing it has proved to be difficult for the MySQL integration test). Can you rebase the PR to fix the conflict?