dheerajturaga commented on PR #70639: URL: https://github.com/apache/airflow/pull/70639#issuecomment-5137074453
Thanks for teasing this apart — I think the direction is right and worth landing. The thing that makes this feel load-bearing rather than tidying: #70147 shipped schema support but no user-reachable knob for the two tables it was actually motivated by, so deployments with a separate result-backend schema get a silently-skipped `db clean` for Celery today. The obvious shortcut — adding `[celery] result_backend_schema` in core — recapitulates the FAB `session` hack exactly (core reading a provider's config at module import with a `fallback` because provider config isn't loaded yet). This PR is the cheap way out of that, and it reuses the `db-managers` shape rather than introducing a new pattern. A few interface points I'd raise, in the order they bother me: 1. **Nameable contract.** The provider callable returns `list[dict]` splatted into `_TableConfig(**spec)` — a leading-underscore dataclass. Any field rename in core silently breaks installed providers with no version negotiation, and `except Exception: logger.warning(...)` swallows it. Either a `TypedDict` for the payload or promoting `_TableConfig` to a public `TableConfig` would give the extension point a stable, type-checkable shape. 2. **Don't silently skip when the user was explicit.** `--tables celery_taskmeta` failing quietly is exactly the failure mode this PR exists to fix. Log-and-skip is reasonable for a bare `db clean`; an explicit table name that resolves to a broken provider callable should be a hard error. 3. **Silent name collisions.** `itertools.chain` puts providers last, so a provider declaring `table_name="task_instance"` overrides core's config with no diagnostic. One `if name in existing: raise` closes it. 4. **`test_no_models_missing` narrows.** With `celery_taskmeta`/`celery_tasksetmeta` gone from `config_list`, the "every ORM model is either cleaned or excluded" guard no longer sees provider-contributed tables. Worth extending it to walk the merged set so a future orphan provider model still trips the check. On the two candidates: - **FAB `session`** — I'd move it. That `fallback="database"` comment in `db_cleanup.py` is the artifact this PR exists to delete; not moving it is the odd choice. Same table, same recency column, resolved at the right layer. - **Edge3** — worth flagging that the Edge executor already purges its own tables in the hot path (`_purge_jobs` in `edge_executor.py`), so `db clean` here is a safety net for orphans (executor not running, team removed, executor swapped out), not the primary reaper. `edge_logs` is the cleanest fit. `edge_job` needs a terminal-state guard via `extra_columns=["state"]` + `extra_filters` — the pattern `connection_test_request` already uses — and `queued_dttm` reads to me as the better recency column since `last_update` is nullable. `edge_worker` I'd leave alone: it's a current-state registry keyed by `worker_name`, not an age-based cleanup target. Neither is urgent; both fall out cleanly once the extension point is in. --- Drafted-by: Claude Code (Opus 5); reviewed by @dheerajturaga before posting -- 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]
