Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 6 additions & 16 deletions .github/workflows/database-sweep.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@
name: Sweep test databases

on:
workflow_run:
workflows: [Test]
branches: [master]
types: [completed]
schedule:
- cron: '0 3 * * *'
workflow_dispatch:
inputs:
dry-run:
Expand All @@ -28,15 +26,10 @@ concurrency:

jobs:
sweep:
# A separate completion workflow also runs after failed or cancelled tests.
if: >-
github.repository == 'pyathena-dev/PyAthena' &&
((github.event_name == 'workflow_run' &&
github.event.workflow_run.event == 'schedule' &&
github.event.workflow_run.head_repository.full_name == github.repository) ||
(github.event_name == 'workflow_dispatch' && github.ref == 'refs/heads/master'))
# Scheduled runs use the default branch; manual dispatch must choose it.
if: github.repository == 'pyathena-dev/PyAthena' && github.ref == 'refs/heads/master'
runs-on: ubuntu-latest
timeout-minutes: 15
timeout-minutes: 60

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round two (claims and operations): CLEAN

  • Base 8679f04a36f6ffd7190b892c32b1938672ab502f, head a1ee0099081d5dcf78a79621fd0d4d4aeca774d4.
  • "A GitHub-hosted job stops after six hours": no Test job sets timeout-minutes; database-sweep.yaml is the only workflow with one. The longest recent Test run, 36526628480, took 4h27m of wall-clock time including reruns, each of which is its own session.
  • "Leftovers live at most about two days": an object created just after the 03:00 UTC run is younger than a day at the next run and is deleted at the one after.
  • Leaks line up with cancellations. Cancelled/total Test runs: 09-20 15/38, 09-21 2/4, 09-23 22/37, 09-24 11/22, from gh run list --created; 09-25 through 09-28 are from the issue. 09-22 had no runs, and 09-27 had no cancellations and left nothing.
  • Backlog counts (1,625 / 5,812 / 538) come from a read-only preview with this head against account 676287850544. The 09-25 and 09-28 totals are from the same inventory, grouped by CreateTime date.
  • Timeout: at about 0.53 s per database (the 09-27 run deleted 690 in 6m9s), the worst day of about 2,800 databases takes about 25 minutes, within 60.
  • AWS operations: the daily API volume is the leak count. Glue metadata calls are not subject to the Athena metadata throttling measured in Investigate persistent metadata throttling during SQLAlchemy reflection聽#780. Retries stay botocore standard mode with 3 total attempts, and the script adds none.
  • Stale prose: no remaining references to seven days or the weekly trigger (git grep).
  • The changes filter skips the SQLAlchemy and Spark suites for this PR; only the PyAthena suite runs on Ready.

permissions:
contents: read
id-token: write
Expand All @@ -49,11 +42,8 @@ jobs:
DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && inputs.dry-run && 'true' || 'false' }}

steps:
# Never execute code or consume artifacts from the triggering test run.
- name: Checkout default branch
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
with:
ref: master
persist-credentials: false

- uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0
Expand Down
3 changes: 2 additions & 1 deletion docs/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ AWS_ATHENA_S3_TABLES_CATALOG=s3tablescatalog/your-table-bucket

Each test process creates its own namespace in the table bucket, named like its schema, and deletes it with any remaining tables at the end; with pytest-xdist that is each worker, not the controller.
The test identity needs `s3tables:CreateNamespace`, `s3tables:DeleteNamespace`, `s3tables:ListTables`, and `s3tables:DeleteTable` on the table bucket.
A session that stops early leaves its namespace behind; `scripts/sweep_databases.py` removes such namespaces once they are more than seven days old.
A session that stops early leaves its namespace behind; `scripts/sweep_databases.py` removes such namespaces once they are more than one day old.
It also removes the database and namespace of a session still running after one day, if they are in the account, region, and table bucket that the sweep covers.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent follow-up (relayed): Codex, read-only and static, on a1ee009..c986038. FINDINGS: the sentence at c986038 said the script removes a long-running local session's resources in the same account. The scheduled sweep uses one fixed region and S3 Tables bucket, so resources in another region or table bucket stay.

Repaired in 8a0e072: the sentence is now limited to "the account, region, and table bucket that the sweep covers". The PR description is amended too. Self-review of the repair: CLEAN. main builds the table-bucket ARN from the configured region and AWS_ATHENA_S3_TABLES_CATALOG, and the Glue client uses the session region. just docs lint passes.

Second independent follow-up (Codex, read-only, static) on c986038..8a0e072, covering docs/testing.md, main/sweep_databases/sweep_s3tables_namespaces, and database-sweep.yaml: CLEAN.


The S3 Tables tests live in `tests/pyathena/sqlalchemy/test_base.py` and `tests/pyathena/test_glue.py` and run under `just test pyathena`, not the SQLAlchemy compliance-suite commands.
Managed storage and S3 Tables tests skip when their respective optional configuration is absent.
Expand Down
45 changes: 29 additions & 16 deletions scripts/sweep_databases.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,27 +13,30 @@
# AWS_PROFILE and AWS_DEFAULT_REGION can select the account and region.
#
# Eligible databases must exactly match a PyAthena or SQLAlchemy fixture name
# and have a creation time more than seven days old. Resource links and federated
# databases are excluded. The script completes inventory before deleting and
# rechecks eligibility and creation time immediately before each deletion.
# Only missing-database errors are ignored; other API failures stop the sweep.
# and have a creation time more than one day old, longer than a CI test session
# can run (a GitHub-hosted job stops after six hours). A local session against
# the same account that stays open longer can lose its database. Resource
# links and federated databases are excluded. The script completes inventory
# before deleting and rechecks eligibility and creation time immediately
# before each deletion. Only missing-database errors are ignored; other API
# failures stop the sweep.
# Deletion removes Glue database and table metadata, not S3 objects.
#
# With AWS_ATHENA_S3_TABLES_CATALOG set (s3tablescatalog/<table-bucket>), the
# script also sweeps that table bucket's namespaces named like PyAthena test
# schemas and more than seven days old, deleting their tables first. Test
# schemas and more than one day old, deleting their tables first. Test
# sessions create and delete such a namespace; a session that stops early
# leaves it behind. The namespace ID is rechecked before its tables are deleted
# and again before the namespace is deleted. Each table is deleted only if it
# still belongs to that namespace, at the version just read; missing tables are
# skipped. DeleteNamespace takes only a name, so an empty namespace recreated
# under the same name right after the last recheck would still be deleted.
#
# .github/workflows/database-sweep.yaml runs this script after scheduled Test
# runs complete on master, including failures and cancellations. It does not run
# after PR tests or manually dispatched tests. Manual sweep dispatch on master
# defaults to preview. The job has a 15-minute timeout; a timeout or API failure
# can leave eligible databases for a later run.
# .github/workflows/database-sweep.yaml runs this script daily on master, so
# while those runs succeed, a cancelled test run's leftovers are removed within
# about two days. Manual sweep dispatch on master defaults to preview. The job
# has a 60-minute timeout; a timeout or API failure can leave eligible databases
# for a later run.

import argparse
import contextlib
Expand All @@ -54,6 +57,8 @@
rf"(?:{_PYATHENA_TEST_SCHEMA}|test_[0-9a-f]{{12}}(?:_test_schema(?:_2)?)?)"
)
_TEST_NAMESPACE = re.compile(_PYATHENA_TEST_SCHEMA)
# Longer than a CI test session can run; see the module comment.
_RETENTION = timedelta(days=1)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review round one (implementation behavior): CLEAN

  • Base 8679f04a36f6ffd7190b892c32b1938672ab502f, head a1ee0099081d5dcf78a79621fd0d4d4aeca774d4.
  • Covered: .github/workflows/database-sweep.yaml (trigger, job condition, checkout, timeout, DRY_RUN), scripts/sweep_databases.py (_RETENTION in both sweeps, docstrings, header), scripts/tests/test_sweep_databases.py, docs/testing.md.
  • Behavior: a scheduled run has github.ref == refs/heads/master and DRY_RUN=false, so it applies. A dispatch from another branch is skipped, and a dispatch on master still defaults to preview. Checkout without ref gets the dispatched or scheduled master commit, and no other run's code or artifacts are involved.
  • Names: only per-session random names match. They come from tests/__init__.py:34 (pyathena_test_ + 10 characters) and tests/sqlalchemy/conftest.py:49-63 (<ident>, _test_schema, _test_schema_2). No fixture reuses a database across sessions, and benchmarks use their own names.
  • Tests: test_databases_expire_after_one_day and test_namespaces_expire_after_one_day run each sweep in preview with a 2-day-old and a 12-hour-old object. Against the old seven-day cutoff, the 2-day cases fail (2 failed). Recheck, pagination, and absence handling are unchanged and stay covered by the existing tests.
  • just lint and just scripts (112 passed): pass.



def _eligible(database: dict[str, Any], cutoff: datetime) -> bool:
Expand All @@ -69,13 +74,21 @@ def _eligible(database: dict[str, Any], cutoff: datetime) -> bool:


def sweep_databases(client: Any, catalog_id: str, *, dry_run: bool = True) -> dict[str, int]:
"""Preview or delete test databases older than seven days.
"""Preview or delete test databases older than one day.

Fixtures generate fresh database names for each session or worker.
Databases younger than seven days are retained, including concurrent CI runs.
Databases younger than one day are retained, including concurrent CI runs.
Only Glue metadata is deleted; S3 objects and child catalogs are untouched.

Args:
client: A boto3 Glue client.
catalog_id: The ID of the Data Catalog to sweep.
dry_run: Only count eligible databases.

Returns:
The numbers of eligible, deleted and skipped databases.
"""
cutoff = datetime.now(UTC) - timedelta(days=7)
cutoff = datetime.now(UTC) - _RETENTION
# Finish pagination before deleting anything from the catalog.
candidates = [
database
Expand Down Expand Up @@ -127,11 +140,11 @@ def _eligible_namespace(namespace: dict[str, Any], cutoff: datetime) -> bool:
def sweep_s3tables_namespaces(
client: Any, table_bucket_arn: str, *, dry_run: bool = True
) -> dict[str, int]:
"""Preview or delete test S3 Tables namespaces older than seven days.
"""Preview or delete test S3 Tables namespaces older than one day.

Test sessions create a namespace named like their schema and delete it when
they finish; this removes the ones a session left behind. Namespaces younger
than seven days are retained, including those of running sessions.
than one day are retained, including those of running sessions.

Args:
client: A boto3 S3 Tables client.
Expand All @@ -141,7 +154,7 @@ def sweep_s3tables_namespaces(
Returns:
The numbers of eligible, deleted and skipped namespaces.
"""
cutoff = datetime.now(UTC) - timedelta(days=7)
cutoff = datetime.now(UTC) - _RETENTION
# Finish pagination before deleting anything from the table bucket.
candidates = [
namespace
Expand Down
20 changes: 20 additions & 0 deletions scripts/tests/test_sweep_databases.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,14 @@ def test_preview_and_apply_finish_pagination_before_mutating(glue):
}


@pytest.mark.parametrize(("age", "eligible"), [(timedelta(days=2), 1), (timedelta(hours=12), 0)])
def test_databases_expire_after_one_day(glue, age, eligible):
client, stubber = glue
database = {**DATABASE, "CreateTime": datetime.now(UTC) - age}
stubber.add_response("get_databases", {"DatabaseList": [database]}, {"CatalogId": CATALOG})
assert sweep_databases(client, CATALOG)["eligible"] == eligible


@pytest.mark.parametrize(
"current",
[
Expand Down Expand Up @@ -255,6 +263,18 @@ def test_only_expired_session_namespaces_are_eligible(properties, expected):
assert _eligible_namespace({**NAMESPACE, **properties}, OLD + timedelta(days=1)) is expected


@pytest.mark.parametrize(("age", "eligible"), [(timedelta(days=2), 1), (timedelta(hours=12), 0)])
def test_namespaces_expire_after_one_day(s3tables, age, eligible):
client, stubber = s3tables
namespace = {**NAMESPACE, "createdAt": datetime.now(UTC) - age}
stubber.add_response(
"list_namespaces",
{"namespaces": [namespace]},
{"tableBucketARN": BUCKET_ARN, "prefix": "pyathena_test_"},
)
assert sweep_s3tables_namespaces(client, BUCKET_ARN)["eligible"] == eligible


def test_namespace_sweep_deletes_tables_then_namespace(s3tables):
client, stubber = s3tables
listing = {"tableBucketARN": BUCKET_ARN, "prefix": "pyathena_test_"}
Expand Down
Loading