aminghadersohi commented on PR #39922: URL: https://github.com/apache/superset/pull/39922#issuecomment-4443751286
**Internal architecture review — findings & resolutions** Ran an independent codex review of the plugin registry design. Summary of findings and their status: | Finding | Severity | Status | |---------|----------|--------| | `_ensure_plugins_loaded()` flag not lock-protected — race condition on concurrent first-calls | MEDIUM | ✅ Fixed in eface3bf — added `threading.Lock` with double-checked locking | | Duplicate `register()` call silently overwrites | MEDIUM | Already handled — `registry.py` lines 68-71 emit `logger.warning` on overwrite, no silent overwrite | | `extract_column_refs()` returns `[]` for unrecognised configs | MEDIUM | Accepted — correct for the 7 known types (plugins always registered); unknown types have no column schema to validate against | | `BaseChartPlugin.extract_column_refs()` no-op default | NIT | Accepted — base class provides a safe default; concrete plugins override for their types | | Local imports in `chart_utils.py` / `dataset_validator.py` | NIT | Already addressed in previous commit with explanatory comments on why they must stay local (circular dependency prevention) | The thread-safety fix (`eface3bf`) is the only code change — the others are either already handled or accepted design decisions. -- 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]
