chore(warehouse-sources): route v2 pipeline post-load through shared orchestrator - #74746
Merged
Conversation
…orchestrator Replace PipelineNonDLT._post_run_operations' inline copy of the post-load sequence with a call to run_post_load_operations, deleting the duplicated compaction, S3 publishing, bookkeeping, table validation, CDC seeding, and view-sync code. The wrapper stays as a method because the e2e tests patch it. Behavior deltas for the v2 pipeline, all aligning it with v3: - CDC-snapshot runs get threshold-based delta maintenance instead of unconditional compaction - gains post-load repartition detection (feature-flag gated) and post-load duration metrics - existing_queryable_folder is read live from schema.table instead of the pipeline-start snapshot - revenue-ready notification now runs before initial_sync_complete is set (independent operations) The duplicated-block error-handling tests are covered by pipelines/common/test/test_load.py; a new test guards the wrapper's return value and forwarded kwargs, which every e2e test mocks past.
|
😎 Merged successfully - details. |
Contributor
|
Hey @estefaniarabadan! 👋 It looks like your git author email on this PR isn't your
You can fix it for this repo with: git config user.email "you@posthog.com"Or set it globally with |
estefaniarabadan
marked this pull request as ready for review
July 29, 2026 14:24
Contributor
|
Reviews (1): Last reviewed commit: "chore(warehouse-sources): route v2 pipel..." | Re-trigger Greptile |
Contributor
Author
|
/trunk merge |
Gilbert09
approved these changes
Jul 29, 2026
…ep list Finish the phase-2 step shape for run_post_load_operations: - Split _register_tables_and_seed into _register_table (plain table validation) and _run_cdc_post_load (companion registration + snapshot seeding), the latter invoked only for CDC schemas and companion writes. - Move the revenue-ready notification, revenue/engineering analytics view syncs, and repartition detection into POST_LOAD_STEPS, a list of callables sharing one PostLoadStep signature — the future registration point for product hooks. Behavior-neutral except: the revenue-ready notification now runs after table registration instead of between last_synced_at and initial_sync_complete (independent operations), and the cdc_seed_check info log no longer fires for non-CDC schemas.
2 tasks
2 tasks
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
The v2 pipeline's
_post_run_operationscarried an inline copy of the shared post-load sequence inpipelines/common/load.py— compaction, S3 publishing, sync bookkeeping, table validation, CDC companion seeding, view syncing. Every fix had to be written twice: #74630 is a recent example, patching the same transient-S3 handling in both copies in one PR. And the shared orchestrator itself still mixed the core publish/register flow with CDC-specific branching and product side effects.This is part 2 of 2 of the post-load unification (following #74671). With this change,
pipelines/common/has a single post-load implementation used by both pipeline versions and CDC companion seeding, with the CDC-specific logic and product side effects behind their own seams.Changes
Commit 1 — route v2 through the shared orchestrator:
PipelineNonDLT._post_run_operationsis now a thin wrapper callingrun_post_load_operations, forwarding the resource and last incremental value (so desc-sort cursor finalization keeps working) and returning the orchestrator's queryable folder (so ducklake'sprepared_queryable_folderkeeps flowing). The ~95-line duplicated block and six now-unused imports are deleted. The wrapper stays a method because the e2e testspatch.objectit as a seam.Commit 2 — finish the orchestrator's step shape:
_register_tables_and_seedis split into_register_table(plain table validation/registration) and_run_cdc_post_load(companion registration + snapshot seeding), the latter invoked only for CDC schemas and companion writes — so the non-CDC path no longer routes through CDC branching.POST_LOAD_STEPS, a tuple of callables sharing onePostLoadStepprotocol signature. This is the seam where product hooks can later register post-load side effects (following theexternal_product_hooks.pypattern) instead of editing this module.Note
Behavior deltas, each aligning v2 with what v3 already does or reordering independent operations:
existing_queryable_folderis read live fromschema.tableinstead of the pipeline-start snapshot (fresher, and correct when partial loading created the table mid-run)last_synced_atandinitial_sync_complete(the operations are independent)cdc_seed_checkinfo log no longer fires for non-CDC schemasHow did you test this code?
pipelines/pipeline_v2/+pipelines/common/: all passing after each commit (43 and 41+15 respectively)uv run mypy --cache-fine-grained .: no issues in 17,083 filesruff check/ruff format: cleanTest changes: deleted
TestPostRunOperationsCompactionErrorHandling— it exercised the deleted duplicated block, and the same regressions (transient S3 blip vs genuine failure, for both the compact and maintenance paths) are covered bypipelines/common/test/test_load.py. Added one test asserting the wrapper returns the orchestrator's queryable folder and forwardsresource/last_incremental_field_value— that regression is otherwise silent: ducklake registration reads the result with.get(...), desc-sort finalization just skips, and every e2e test mocks_post_run_operations, so no existing test would notice. The commit-2 restructure adds no new branches, so no new tests.Automatic notifications
Docs update
No user-facing or documented-workflow changes.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored with Claude Code as phase 2 (part 2 of 2) of the warehouse pipelines separation plan, continuing #74352 and #74671. Skills invoked: /writing-tests (gating the test deletion and the one new test). Verification: repo-wide mypy, targeted
hogli testruns, ruff. Decisions: kept_post_run_operationsas a method rather than inlining intorun()because the e2e suite patches it viapatch.object;POST_LOAD_STEPSis a plain module-level tuple rather than a registry — the registration mechanism is deliberately out of scope per the plan, this commit only shapes the entries to a shared signature; the behavior deltas listed above were accepted deliberately rather than parameterized away, since each matches the v3 path the orchestrator already serves.