Skip to content

Let providers contribute tables to 'db clean' - #70639

Open
uranusjr wants to merge 1 commit into
apache:mainfrom
astronomer:db-cleanup-provider-contributed-tables
Open

Let providers contribute tables to 'db clean'#70639
uranusjr wants to merge 1 commit into
apache:mainfrom
astronomer:db-cleanup-provider-contributed-tables

Conversation

@uranusjr

@uranusjr uranusjr commented Jul 28, 2026

Copy link
Copy Markdown
Member

This is the follow-up mentioned in #70147.

A 'db-cleanup-tables' key in provider.yaml is added to specify a callable returning cleanup configs, discovered via ProvidersManager. The 'db clean' command merges them into the list of tables to clean at runtime.

Celery uses it to register 'celery_taskmeta' and 'celery_tasksetmeta' tables, using the '[celery] result_backend_schema' config that schema-qualifies them when the result backend is in its own schema. This allows them to always be correctly found by the 'db clean' command. Both are removed from Core's hardcoded list.

Possible future candidates (no Core change needed, deferred to their maintainers): FAB's session table and the edge3 tables.

This adds a 'db-cleanup-tables' key in provider.yaml to specify a
callable returning cleanup configs, discovered via ProvidersManager.
The 'db clean' command merges them into the list of tables to clean at
runtime.

Celery uses it to register 'celery_taskmeta' and 'celery_tasksetmeta'
tables, using the '[celery] result_backend_schema' config that
schema-qualifies them when the result backend is in its own schema. This
allows them to always be correctly found by the 'db clean' command.
Both are removed from Core's hardcoded list.
@uranusjr
uranusjr force-pushed the db-cleanup-provider-contributed-tables branch from 6e2609b to 1ffe313 Compare July 29, 2026 05:46
@dheerajturaga

Copy link
Copy Markdown
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants