Seperately configure enabling CDC store for ClickHouse destination#4366
Seperately configure enabling CDC store for ClickHouse destination#4366jgao54 wants to merge 3 commits into
Conversation
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
| return nil, err | ||
| } | ||
|
|
||
| dstPeerType, err := connectors.LoadPeerType(ctx, a.CatalogPool, config.DestinationName) |
There was a problem hiding this comment.
this introduces an additional query per batch, which is a bit wasteful given destination type does not change during the course of a CDCFlowWorkFlow. but the query is relatively cheap.
Alternatively we can pass in DestinationType to FlowConnectionConfigCore, but it is a bit redundant and we also have to worry about backward compatibility handling for existing workflows.
There was a problem hiding this comment.
Query per batch seems like a high cost to me for what it does (we're already hitting pool exhaustion on overloaded instances and #4256 is helping us counteract that). Maybe a flow env could be a place that can be set once then queried from pg connector creation and remembered as a connector field?
Overall my opinion is still that overriding the existing setting statically depending on the deployment is the lowest-code solution.
c474508 to
7b4078f
Compare
| DefaultValue: "true", | ||
| ValueType: protos.DynconfValueType_BOOL, | ||
| ApplyMode: protos.DynconfApplyMode_APPLY_MODE_IMMEDIATE, | ||
| ApplyMode: protos.DynconfApplyMode_APPLY_MODE_AFTER_RESUME, |
There was a problem hiding this comment.
since we are fetching this value on the activity level, no need for apply immediate as it would not take effect anyways
There was a problem hiding this comment.
This one would be fetched on the next batch? Although I'm all for moving it to activity-level
There was a problem hiding this comment.
IMMEDIATE skips setting it in the env map so it would always fetch from db, but with AFTER_RESUME it will just return what's in the env (after it gets fetched/set the first time).
that env gets "reset" on a new activity, which can be achieved with pause/resume.
i guess another way to handle this is just directly populate it into config.Env ourselves, also a bit hacky
| DefaultValue: "true", | ||
| ValueType: protos.DynconfValueType_BOOL, | ||
| ApplyMode: protos.DynconfApplyMode_APPLY_MODE_IMMEDIATE, | ||
| ApplyMode: protos.DynconfApplyMode_APPLY_MODE_AFTER_RESUME, |
There was a problem hiding this comment.
This one would be fetched on the next batch? Although I'm all for moving it to activity-level
| return a.Alerter.LogFlowError(ctx, config.FlowJobName, fmt.Errorf("failed to load destination peer type: %w", err)) | ||
| } | ||
|
|
||
| cdcStoreEnabled, err := internal.PeerDBCDCStoreEnabledForDestination(ctx, config.Env, destinationType) |
There was a problem hiding this comment.
Maybe I'm missing something, but if the CDC store is PG-only concern, why not read the config on the connector construction and store as a field on the connector?
There was a problem hiding this comment.
hmmm, taking a look..
There was a problem hiding this comment.
updated.
I also adjusted the code to determine the destination type and update the config.Env in the SyncFlow itself, such that the default value is destination-specific, but explicitly setting the env can still be honored. This is also given PG-to-PG and PG-to-CH will be different services so a single env is cleaner than having a separate one for PG-to-CH.
There was a problem hiding this comment.
Was thinking of something like this https://github.com/PeerDB-io/peerdb/compare/cdc-store-maybe-cleaner?expand=1
There was a problem hiding this comment.
okay, let's go with that, it's got a bit less magic which seems like a good thing.
8e9dad4 to
bc84902
Compare
|
❌ Test FailureAnalysis: A deterministic compile error — postgres_source.go:277 references a non-existent field CDCStoreEnabled on PostgresCDCConfig, so the test binary never built (not a flaky test). |
bc84902 to
1c917b5
Compare
❌ Test FailureAnalysis: Not flaky — a deterministic Go compile error (unknown field CDCStoreEnabled in PostgresCDCConfig at postgres_source.go:277) failed the build before any tests ran. |
1c917b5 to
ba32dab
Compare
❌ Test FailureAnalysis: A deterministic Go compilation error ("declared and not used: cdcStoreEnabled" in connectors/postgres/postgres_source.go:255) prevented the test binary from building, so this is a real code bug, not a flaky test. |
❌ Test FailureAnalysis: All three flow_test matrix jobs hit the same 20-minute e2e timeout with every CDC test across every destination hanging on un-normalized data, a deterministic regression matching PR #4366's change to the shared CDC store rather than a flaky failure. |
ba32dab to
0ede60e
Compare
❌ Test FailureAnalysis: All three e2e shards hit the 20-minute package timeout with ClickHouse-destination tests stuck, matching this PR's change to ClickHouse CDC-store behavior, indicating a real regression that hangs CDC mirrors rather than a flaky failure. |
0ede60e to
a9efd94
Compare
❌ Test FailureAnalysis: Not flaky: Test_Kafka's TestSimple/TestDefault/TestMessage deterministically fail with empty output (actual "") across all three matrix shards, indicating the PR's CDC-store enablement change stopped data reaching the Kafka destination, with the 20m package timeout being a downstream symptom. |
a9efd94 to
cd71e2d
Compare
Building on #4366 - Introduce `PEERDB_CLICKHOUSE_CDC_STORE_ENABLED` to allow separating handling of cdc store enablement for ClickHouse (since other destinations like PG-to-PG and PG-to-Snowflake requires this to be enabled). - Still honor existing services with CDC store disabled, to avoid OOM issues resurfacing - Currently `PEERDB_CLICKHOUSE_CDC_STORE_ENABLED` is default to `true`, can be updated to `false` once we are ready to flip the switch (PR is currently a no-op) --------- Co-authored-by: Joy Gao <[email protected]>
2 participants
PEERDB_CLICKHOUSE_CDC_STORE_ENABLEDto allow separating handling of cdc store enablement for ClickHouse (since other destinations like PG-to-PG and PG-to-Snowflake requires this to be enabled).PEERDB_CLICKHOUSE_CDC_STORE_ENABLEDis default totrue, can be updated tofalseonce we are ready to flip the switch (PR is currently a no-op)UPDATE:
PEERDB_CDC_STORE_ENABLED, but with default value destination-specific.PEERDB_CDC_STORE_ENABLEDdefault to false for ClickHouse as destination. So we can hold off merging this PR for now until we are ready for this change.