rusackas commented on PR #40237: URL: https://github.com/apache/superset/pull/40237#issuecomment-4524126963
Thanks for the thorough review @aminghadersohi! All four addressed in 05557350ab: - ✅ **HIGH (thread safety)**: added `_ENGINE_CACHE_LOCK = threading.Lock()`, wrap both lookup and write. The 'one engine per process per (id, URL, kwargs)' guarantee is now strict. - ✅ **MEDIUM (`self.id is None`)**: skip caching when `self.id is None` so distinct unsaved instances don't collide. Added a second test `test_get_sqla_engine_does_not_cache_unsaved_instances` covering this. - ✅ **NIT (type annotation)**: tightened `dict[tuple[Any, ...], Engine]` → `dict[tuple[int, str, str], Engine]`. - ✅ **NIT (walrus)**: `if cached := _ENGINE_CACHE.get(cache_key): return cached`. - ⏭️ **NIT (file-wide inline import cleanup)**: agreed, follow-up — out of scope for this PR. Also addressed @sadpandajoe's blocker (the cache was gated by `if not nullpool:` but every production callsite uses the `nullpool=True` default, so the cache was dormant in prod) by removing the gate. See his thread for details. Updated the regression test to use the default-arg path so it actually exercises the production code path. Pre-commit (ruff/mypy/pylint) all pass locally. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
