fix(data-imports): detect a hollow delta table on the sync path - #75767
Draft
posthog[bot] wants to merge 1 commit into
Draft
fix(data-imports): detect a hollow delta table on the sync path#75767posthog[bot] wants to merge 1 commit into
posthog[bot] wants to merge 1 commit into
Conversation
A sync that 404s on a parquet file the live delta log still references had no self-heal: detection lived only in the repartition scan, so the job failed, failed again on every retry, and re-enabling the schema changed nothing. Reuse the existing repartition detection helpers from the pipeline's failure path so the sync arms `delta_revive_required` and the next run's handle_corrupted_delta_log resets and rebuilds from source, non-billable. Also stop vacuum from creating that state in the first place: it passed retention_hours=24 with enforce_retention_duration=False, which deliberately disabled delta-rs's retention guard and let it physically delete files a concurrent snapshot, an interleaved repartition swap, or a zombie activity attempt still referenced. Retention now follows the table's own configured window with the guard enforced. Generated-By: PostHog Code Task-Id: dc0b33d2-08a1-4289-95d3-839cf9ae3270
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_missing_live_object_path/_live_missing_data_file).handle_corrupted_delta_logfires on an unreadable_delta_logor adelta_revive_requiredmarker only the repartition path ever sets. A sync that hits the 404 itself has no equivalent check.enforce_retention_duration=False, which deliberately turns delta-rs's retention guard off, so a file can be physically deleted while a concurrent snapshot, an interleaved repartition swap, or a zombie Temporal activity attempt still references it. Vacuum has been rolling out to a lot more tables, and revives have been clustering on a small number of schemas that get torn down and rebuilt repeatedly.Changes
Arm the revive from the sync's failure path. Both pipelines now call
schedule_revive_if_table_is_hollowwhen the run raises. It reuses the two repartition helpers unchanged, so there is one definition of "hollow" rather than two:_missing_live_object_path__repartitionedtemp)? Returns early for everything else, so a normal failure costs nothing._live_missing_data_fileBoth true, and it sets the
delta_revive_requiredmarker. The error still propagates - this run is already past the point where the revive would have run, so the next run'shandle_corrupted_delta_logdoes the reset and non-billable rebuild.Stop vacuum making hollow tables. Retention now follows the table's own
deletedFileRetentionDuration(a week, by default) with the guard enforced:Note
The trade-off is slower reclamation: dead files now linger up to the configured retention instead of 24 hours. Worth it against a failure mode whose only recovery is a full rebuild from source. If we want the space back sooner, the right lever is lowering
deletedFileRetentionDurationon the table so the guard stays honest, not disabling the guard.Complementary to #75240 and #75242, which classify racy file-not-found errors on the maintenance paths as transient. Those retry past the symptom; a genuinely hollow table stays undetected. This PR adds the detection and removes the cause. No file overlap beyond
delta_table_helper.py, and different functions within it.How did you test this code?
Automated tests I added, in
test_extract.py::TestScheduleReviveIfTableIsHollow:reason="sync_missing_data_file". This is the customer's failure, and no existing test covers detection outside the repartition path.__repartitionedtemp sibling both return early without touching the schema, and without paying for a live-log scan.I could not run the pytest suite in this environment: it has no Docker and only the Postgres client libraries, so
django_dbtests error on connect. What I did run:What actually ran
uv run mypy --cache-fine-grained .repo-wide:Success: no issues found in 17251 source files.ruff check/ruff formatandhogli ci:preflight --fix: clean.pytest .../test_delta_table_helper.py -k vacuum: 8 passed (these do not need the DB).schedule_revive_if_table_is_hollowdirectly in a Django shell against mocks for all four branches above, confirming the marker payload and that the verification call is skipped on the early-return paths.deltalakethatvacuum's defaults areretention_hours=None, enforce_retention_duration=True, so dropping the arguments is exactly "use the table's window, enforce the guard".Automatic notifications
Docs update
No user-facing surface changes, so nothing to update.
🤖 Agent context
Autonomy: Fully autonomous
Written by Claude (Opus) via PostHog Code from a Signals inbox report. Skills invoked:
/writing-tests,/writing-code-comments.Decisions along the way:
start_to_close_timeoutreaches a week on some paths, so no constant is obviously safe). Deferring to the table's configured window with the guard on is simpler and keeps the safety property rather than re-picking a number that can go stale.pipeline_v2andpipeline_v3rather than into the delta write helper, so it also catches a 404 raised from a read or from post-load work, not just from the merge.Created with PostHog Desktop from this inbox report.