ref(etl-api): deprecate POST update-publication endpoint#743
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 551ddf1378
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Ok(HttpResponse::Ok().finish()) | ||
| Ok(HttpResponse::Ok() | ||
| .insert_header(("Deprecation", "true")) |
There was a problem hiding this comment.
Use an RFC-compliant Deprecation header value
For clients that parse the standardized HTTP Deprecation field, this header will fail to parse because RFC 9745 defines it as an Item Structured Header whose value MUST be a date, e.g. @1688169599 (see https://www.ietf.org/rfc/rfc9745.html#section-2.1); true was only allowed by older drafts. Since this change adds the runtime deprecation signal for the endpoint, use an sf-date for the effective deprecation date, or omit Deprecation and rely on the Link header until a date is known.
Useful? React with 👍 / 👎.
…base#739) Wrap the test destination in TestDestinationWrapper and replace the SELECT polling loops with wait_for_events_count. Touches 9 streaming tests; removes 5 polling helpers.
* feat(error): add DestinationTimeout error kind * feat(clickhouse): add ClickHouseClientConfig and thread through Adds a sibling config struct to ClickHouseInserterConfig with per-operation server-side timeout fields and a client_timeout_epsilon. Threaded through ClickHouseDestination::new and ClickHouseClient::new; all existing callers pass Default::default(). No behavior change yet: the new config field is plumbed but not consulted. * feat(clickhouse): wrap client calls in tokio::time::timeout Adds TimeoutOp + timeout_call so each ClickHouseClient call is bound by client_budget(server_budget) = server + epsilon. Server-side options (connect_timeout, http_connection_timeout, http_send_timeout, http_receive_timeout) are set globally; max_execution_time and lock_acquire_timeout are applied per-call to DDL/TRUNCATE so they do not leak into probe and schema_query budgets. Client-side deadlines surface as DestinationTimeout. * test(clickhouse): cover client_budget and timeout_call Covers ClickHouseClientConfig::client_budget arithmetic, TimeoutOp::Display matching the strings used in error messages, and timeout_call across its three outcomes (deadline expired, inner error, success). Uses tokio::test(start_paused = true) so the deadline test does not block on real wall-clock time. * ref(clickhouse): rename probe timeout, tighten config docs Renames probe_server_timeout to connectivity_check_timeout and shortens the doc-comments on ClickHouseClientConfig per review. Drops the epsilon default from 5s to 4s. * ref(clickhouse): rename TimeoutOp::Probe, drop timeout_kind() Renames TimeoutOp::Probe to ConnectivityCheck (matching the connectivity_check_timeout config field) and updates its Display string to 'connectivity check'. Inlines ErrorKind::DestinationTimeout in timeout_call and drops the now-trivial timeout_kind() helper. * ref(clickhouse): let TimeoutOp drive the server budget Adds TimeoutOp::server_budget, makes timeout_call take a &ClickHouseClientConfig and derive the budget internally via config.client_budget(op.server_budget(config)). Call sites lose their hand-rolled budget bindings and the (op, budget) pairing is now impossible to mismatch. TimeoutOp's three responsibilities (server budget, failed kind, Display label) are documented in one place. * ref(clickhouse): move TimeoutOp into core as ClickHouseOperationKind Renames TimeoutOp to ClickHouseOperationKind and moves it next to ClickHouseClientConfig in core.rs. server_budget and the budget-form client_budget move onto ClickHouseClientConfig (config now takes an op directly), so call sites read 'config.client_budget(op)' instead of the awkward 'config.client_budget(op.server_budget(config))'. failed_kind and Display stay on the enum. Doc-comment reworded to lead with 'Categories of ClickHouse client calls' per review. * ref(clickhouse): tighten doc-comments per review Shortens server_budget, ClickHouseOperationKind, and timeout_call doc-comments. The 'client-side deadlines map to DestinationTimeout' note moves from the enum to timeout_call where the mapping happens. * ref(clickhouse): reword ClickHouseOperationKind doc 'Enables the following:' -> 'Used to:'; bullets shift from gerunds to imperative verbs. * fix(clickhouse): retry DestinationTimeout, restore table context Adds DestinationTimeout to the auto-retry arms in both workers/policy.rs and state/table.rs so client-side timeouts retry on a timed delay instead of falling into the default Manual retry branch. Without this, a single transient timeout would pause replication until an operator intervened. Extends timeout_call with an optional context string and threads table_name through table_columns, truncate_table, and insert.end(), restoring diagnostic info that the previous timeout refactor dropped. Also rewords two stale 'probe and schema_query' comments and adds a unit test for secs_string covering the subsecond floor and zero preservation. * ref(clickhouse): drop redundant comment in client constructor The four .with_option calls have self-explanatory names; the rationale for per-call overrides lives at the per-call sites. * ref(clickhouse): drop _server_ infix from config fields, dedupe error detail Renames schema_query_server_timeout -> schema_query_timeout, ddl_server_timeout -> ddl_timeout, insert_server_timeout -> insert_timeout (and matching constants) for symmetry with connectivity_check_timeout. The doc-comment already says 'server-side', so the infix was redundant. Also drops the leading 'ClickHouse ' from timeout_call's detail format; the static description already begins with 'ClickHouse call', so the rendered output no longer duplicates the prefix. * test(clickhouse): cover inner-error path with context Adds the missing matrix cell: inner ClickHouse error in timeout_call with Some(context). Asserts that the context (e.g. 'table: users') shows up in the detail alongside the per-op kind. * test(clickhouse): assert inner clickhouse error is attached as source Pins that timeout_call's inner-error arm calls etl_error! with 'source: err'. Catches a regression where the source: attribute gets dropped silently, which would lose the underlying clickhouse error from log/debug output. * ref(clickhouse): floor secs_string at 1s ClickHouse interprets 0 as no timeout for http_*_timeout, max_execution_time, and lock_acquire_timeout. Preserving Duration::ZERO as '0' silently disabled the server-side bound while the client-side tokio::time::timeout still fired at the epsilon, so the operator's intent (if any) was overridden anyway. Floor sub-second values at 1s instead and document the behaviour. * fix(clippy): drop needless borrows and reassign-with-default CI clippy (1.93) flagged: - needless_borrows_for_generic_args on four with_option calls: drop & on the secs_string(...) String, with_option takes Into<String>. - field_reassign_with_default in client_budget test: switch to struct update syntax with ..Default::default(). * ref(clickhouse): shorten floor_secs name, space after inner fn floor_secs_string -> floor_secs (return type already says String). Blank line after the inner detail() fn body inside timeout_call. * ref(clickhouse): drop superfluous comment in execute_ddl The per-call .with_option pattern is self-explanatory from the code itself and repeats in truncate_table / table_columns. * test(clickhouse): add GIVEN/WHEN/THEN doc-comments to new tests Documents the 10 unit tests added for ClickHouseClientConfig, ClickHouseOperationKind, timeout_call, and floor_secs in the style used by the integration tests in tests/clickhouse/pipeline.rs. * ref(clickhouse): tighten floor_secs doc-comment * ref(clickhouse): add max_execution_time to validate_connectivity For consistency with the other wrapped calls (execute_ddl, table_columns, truncate_table), bound SELECT 1 with a server-side max_execution_time set to connectivity_check_timeout. Defense in depth against a wedged server's query queue. * ref(clickhouse): rename server/client_budget to server/client_timeout_for server_budget / client_budget on ClickHouseClientConfig were the only methods using the 'budget' vocabulary; everything else (config fields, ClickHouse settings, local idioms) speaks in 'timeout'. Rename to server_timeout_for(op) / client_timeout_for(op) for consistency, plus the matching internal call, doc-comment, local var, and test names.
…se#721) * experimental(ducklake): add support for external maintenances Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * update default values Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * fmt Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * improve behavior and add more tests Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * add logs * lint Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * fix fmt Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * fix tests Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * move ducklake maintenance binary Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * add more timeouts Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * remove in process maintenances Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * fix fmt Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * don't create useless CR when updating the pipeline Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * don't enable expire snapshot by default Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * change kubernetes app type for maintenance jobs Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * add docs for maintenance config Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> --------- Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com>
* feat(clickhouse): thread engine through destination config
Adds `ClickHouseEngine` (`merge_tree` | `replacing_merge_tree`) in
`etl-config::shared`, defaulting to `ReplacingMergeTree`. Wires the
field through `DestinationConfig::ClickHouse`,
`DestinationConfigWithoutSecrets::ClickHouse`,
`ClickHouseInserterConfig`, and the replicator init path.
No behavioral change: the destination still emits `MergeTree` DDL.
Subsequent commits will branch DDL, encoding, and validation on the
engine.
* feat(clickhouse): RMT DDL builders and `__current` view
Adds `create_replacing_merge_tree_sql` (PK-required, ordered by
`primary_key_ordinal_position`, trailing `_etl_lsn` + `_etl_deleted`)
and `create_current_view_sql` (FINAL + tombstone filter, exposes only
user columns). Renames `build_create_table_sql` to
`create_merge_tree_sql` and adds a `create_table_sql` dispatcher.
Wires `create_table_with_metadata` and `recover_applying_metadata`
through the dispatcher and emits the companion view under RMT.
`expected_clickhouse_column_names` is now engine-aware via
`trailing_cdc_column_names`.
Adds `DdlKind::CreateView` for telemetry. No row-encoding changes yet;
RMT tables cannot be written end-to-end until the next commit.
* ref(clickhouse): polish RMT DDL helpers from review
- Rename `issue_create_table_ddl` to `issue_create_table_stmt`.
- Reword CDC/etl column constant doc-comments to lead with the engine.
- Simplify `expected_clickhouse_column_names` to use a chained iterator.
- Add blank lines around the early-return and final `Ok(format!)` in
`create_replacing_merge_tree_sql`.
* feat(clickhouse): RMT encoder and validation
Row encoding now branches on engine via `append_cdc_columns`:
- MergeTree (unchanged): `cdc_operation` + `cdc_lsn` (commit_lsn).
- ReplacingMergeTree: `_etl_lsn` (start_lsn, the RMT version) + `_etl_deleted`
tombstone. `start_lsn` is globally monotonic, so multi-event same-commit
transactions tie-break correctly under `FINAL`.
`PendingRow` carries both `commit_lsn` and `start_lsn`. Initial-copy rows
under RMT encode with `_etl_lsn = 0` so any streaming event wins on FINAL.
`validate_replica_identity_for_clickhouse` now takes the engine and rejects
PK-less schemas under RMT in addition to the existing replica-identity gate.
Adds `ClickHouseClient::server_version` and `ClickHouseDestination::
validate_engine_support`. `new` stays sync and side-effect free; callers
(replicator, example, test_utils) invoke `validate_engine_support` after
construction to surface RMT-on-old-CH (< 23.5) at startup. `UInt8` is
added to the encoder for the new tombstone column.
No integration-test parameterization yet; existing MergeTree tests keep
their current behavior.
* ref(clickhouse): polish RMT encoder helpers from review
- Add blank lines around the early-return and trailing `Ok(())` in
`ensure_engine_supported`.
- Collapse the `ClickHouseValue::UInt8` doc comment to one line.
* test(clickhouse): parameterize spine over both engines + RMT-only tests
Every shared spine test in `pipeline.rs` now runs against both engines
via an inner `..._inner(engine: ClickHouseEngine)` async fn plus two
`#[tokio::test]` wrappers. Assertions are on current state read through
a new `current_state_query` helper in `tests/support/clickhouse.rs`,
which produces engine-appropriate SQL (MT: `LIMIT 1 BY (pks)` then drop
DELETE rows; RMT: `FINAL` + `_etl_deleted = 0`).
Adds `pipeline_rmt.rs` with RMT-only behaviors:
- PK-less source rejection.
- Same-LSN multi-event tx: INSERT+UPDATE collapses to the UPDATE under
`FINAL` (confirms `start_lsn` tie-breaks).
- Same-LSN tx: DELETE+INSERT yields the post-INSERT row.
- `__current` view exposes user columns and reads current state.
- Composite PK ORDER BY honors `primary_key_ordinal_position`.
- Initial-copy then streaming UPDATE: streamed value wins.
- `OPTIMIZE FINAL CLEANUP` physical removal (skipped gracefully when
the server has the experimental gate disabled, which varies by CH
version).
Moves `sequential_transactions_preserve_commit_order` to
`pipeline_mt.rs` because the cdc_lsn-ordering assertion has no analog
under RMT (FINAL collapses the event log).
Adds `ClickHouseTestDatabase::build_destination_with_engine`. The
`column_types` filter now excludes both engines' trailing CDC columns.
`install_crypto_provider` moves to the shared support module so all
three test files share a single Once.
* ref(tests): polish RMT/MT test structs and assertions from review
- `EventLogRow`: drop `#[allow(dead_code)]`; add an `id` assertion that
uses the field meaningfully alongside the CDC columns.
- `IdRow`: replace with a `CountRow` against `SELECT count() AS count`,
so the OPTIMIZE CLEANUP assertion no longer leans on an unused field.
- PK-less rejection test: move the assertion into a real THEN block
that reads the Errored phase reason and checks it mentions the PK
requirement.
- Reword the same-tx DELETE+INSERT assertion to "current state shows the
re-inserted row, not the tombstone" -- clearer than "post-INSERT wins".
* feat(clickhouse): engine-mismatch detection and PK ALTER guard under RMT
Adds two destination-side safety checks:
- `ensure_engine_matches`, called from `ensure_table_exists` on cache
miss, queries the new `ClickHouseClient::table_engine` against
`system.tables` and hard-errors when an existing CH table's engine
differs from the configured engine. Catches the "operator flipped
`engine: merge_tree` to `replacing_merge_tree`" misconfiguration
before any RowBinary INSERT can mis-align.
- `reject_pk_alters_under_rmt`, called from `apply_schema_diff`, refuses
`columns_to_remove` or `columns_to_rename` entries that touch a
primary-key column under RMT. RMT's `ORDER BY` is the source PK; a
silent drop or rename would corrupt dedup.
Unit tests cover the PK-ALTER guard (drop/rename, PK vs non-PK).
Integration tests cover engine mismatch in both directions: MT then
RMT, and RMT then MT. Both expect the table to land in the Errored
phase with a reason naming both engines.
* ref(clickhouse): polish engine-mismatch and PK ALTER guard from review
- `ensure_engine_matches`: tighter logic flow (probe first, then compare)
and a shorter doc comment.
- `apply_schema_diff` RMT-PK-guard comment: explain why `ALTER TABLE`
cannot rewrite the RMT ORDER BY expression, so a PK drop or rename
would leave the table referring to a column that no longer exists.
- Reword "would change the ORDER BY" -> "would invalidate the ORDER BY"
in both the doc comment and the user-facing error message.
- Drop wrapping `---` on multi-line GIVEN/WHEN markers in
`pipeline_rmt.rs`; the visual frame only reads well on single-line
markers.
* docs(clickhouse): document engine flag, RMT behavior, MT path, migration
Adds a `--clickhouse-engine` CLI flag to the etl-examples ClickHouse
binary (snake_case values that mirror the YAML config: `merge_tree` and
`replacing_merge_tree`, default `replacing_merge_tree`) and threads it
through `ClickHouseInserterConfig`.
Rewrites the ClickHouse section of `crates/etl-examples/README.md`:
- Documents both engines with a quick-reference table.
- Explains the ReplacingMergeTree layout (`_etl_lsn`, `_etl_deleted`),
the auto-generated `<table>__current` view, the FINAL read pattern,
and `OPTIMIZE ... FINAL CLEANUP` guidance for operators.
- Documents the MergeTree event-log layout and the latest-event-per-PK
read pattern (`LIMIT 1 BY` + drop DELETEs).
- Adds a migration note for pre-RMT pipelines: drop the CH tables and
re-sync under RMT, or pass `--clickhouse-engine merge_tree` to keep
the previous behavior.
- Calls out the CH >= 23.5 requirement for RMT.
* ref(clickhouse): polish engine-name helper + RMT comment from review
- Move the ClickHouse engine-string lookup onto the type itself as
`ClickHouseEngine::as_clickhouse_str() -> &'static str` in etl-config.
Drops the awkward `engine_to_clickhouse_name` free function in
`core.rs` and the duplicate helper in `pipeline_rmt.rs`. The method
documents the distinction from the snake_case YAML/CLI form.
- Reword the `reject_pk_alters_under_rmt` docstring: drop the
parenthetical "(that's how `CREATE TABLE` was emitted)" and replace
it with the actual SQL we emit, `CREATE TABLE ... ENGINE =
ReplacingMergeTree(...) ORDER BY (<pk cols>)`. Makes the reasoning
concrete rather than abstract.
* ref(clickhouse): move RMT version requirement onto the engine type
Adds `ClickHouseEngine::min_server_version() -> Option<(u32, u32)>` in
etl-config, returning `Some((23, 5))` for RMT and `None` for MT. Drops
the `MIN_REPLACING_MERGE_TREE_VERSION` free constant in core.rs and
reshapes `ensure_engine_supported` to be the error-construction shell
around `engine.min_server_version()`.
The version requirement is a property of the engine variant itself, so
it belongs on the type. core.rs keeps the `EtlResult` plumbing because
etl-config doesn't (and shouldn't) take a dependency on the etl error
infrastructure.
The mismatch error message now names the configured engine literally
(via `as_clickhouse_str()`) rather than hard-coding "ReplacingMergeTree"
in the text, so the message stays correct if a future engine variant
gets its own version floor.
* docs(clickhouse): clarify docs
* ref(clickhouse): clarify RMT DDL docstring + extract PK helper
* ref(clickhouse): use is_some_and for PK rename lookup (clippy)
* fix(clickhouse): refresh RMT current view on schema changes
* fix(api): preserve ClickHouse engine config
* doc(mdtable): format markdown table
* ref(clickhouse): RMT version column = packed EventSequenceKey (UInt128)
Replaces the RMT `_etl_lsn UInt64` version column with `_etl_version
UInt128`, holding the packed source `EventSequenceKey` in the form
`(commit_lsn << 64) | tx_ordinal`. This mirrors the codebase's canonical
event-ordering primitive and gives the RMT version column a true total
order across all events, rather than depending on the inferred invariant
that `start_lsn` is unique per row touch (which holds in stock Postgres
today but isn't a documented guarantee).
- `PendingRow` carries `tx_ordinal: u64` instead of `start_lsn: PgLsn`.
- `append_cdc_columns` computes the packed UInt128 for RMT via a new
`etl_version_value` helper.
- `ClickHouseValue::UInt128(u128)` added; `rb_encode_value` emits the
value as 16-byte little-endian.
- `create_replacing_merge_tree_sql` emits `_etl_version UInt128` and
`ENGINE = ReplacingMergeTree(_etl_version, _etl_deleted)`.
- Initial-copy rows use `commit_lsn = 0, tx_ordinal = 0`, so the packed
version is 0 and any streamed event wins.
- `column_types` test helper filter, RMT integration docstrings, README
and example CLI doc updated for the new column name + encoding.
* test(clickhouse): rename pipeline_{mt,rmt}.rs to long-form engine names
* ref(clickhouse): expand MT/RMT abbreviations to MergeTree/ReplacingMergeTree
* ref(clickhouse): use EventSequenceKey::as_u128 + trim destination doc
- Add `EventSequenceKey::as_u128()` in etl, returning the canonical packed
form (`commit_lsn` in the high 64 bits, `tx_ordinal` in the low 64 bits).
The packing now lives next to the type it describes.
- Drop the local `etl_version_value` helper in the ClickHouse destination
and call `EventSequenceKey::new(...).as_u128()` directly.
- Trim the `ClickHouseDestination` doc comment. Engine-specific layout
belongs on `ClickHouseEngine` (where it already lives); the destination
struct's doc just points at the engine type.
…e#755) * fix(api): do not create k8s labels bigger than 63 characters Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * change suffix Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> --------- Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com>
…#757) Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com>
* fix(ducklake): improve expireSnapshot maintenance Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> * increase duckdb timeout for maintenance runner Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> --------- Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com>
* feat: error and config * feat: implement snowflake auth * feat(snowflake): DDL SQL client * feat(snoflake): implement RowBatch * feat(snowflake): streaming client * update schema encoding * feat(snowflake): ChannelHandler abstraction * feat(snowflake): refactor streaming client * feat(snowflake): minor fixes * feat(snowflake): batching and backpressure * refactor(snowflake): mock tests cleanup * feat(snowflake): example file * chore: rebase * refactor: move channel into streaming module * refactor: update sql client * feat(snowflake): consolidated Snowflake client * feat(snowflake): destination core * test(snowflake): destination tests * test(snowflake): improve polling * test(snowflake): more integration tests * feat(examples): snowflake monitoring ui * feat(examples): snowflake load generator * refactor(snowflake): make mod prepended imports * fix(snowflake): deprecated method * feat(snowlake): reconnect on expired jwt token * refactor(snowflake): feature gate snowflake deps in examples * fix: cargo sort * refactor(snowflake): swap compromised dependency * chore(snowflake): clippy * chore(snowflake): fmt * fix: capitalize errors * test(snowflake): add schema evolution tests for rename, drop, and type change * fix(snowflake): prevent terminal corruption on F2/F3 errors * fix(snowflake): pr review comments * fix: rest client and batch fixes * fix: http client with timeouts * fix: some more comments * fix: fmt * fix: remove warehouse * fix: codeql comments * fix: more comments * rebase * fix: explicit clone * fix: clippy * fix: codeql comment
* feat(xtask): add fmt, check, fix, and msrv commands Port shell scripts to xshell-based xtask commands: - fmt: nightly rustfmt with --check flag - check: pre-PR gate running fmt, sort, clippy, nextest - fix: auto-fix with clippy --fix, fmt, sort - msrv: verify MSRV consistency with --verify flag for full compilation check Also adds cargo-msrv presence check with actionable install instructions. * refactor(xtask): no nextest on check * fix: eprintln * feat: passthough other scripts * fmt
What kind of change does this PR introduce?
Refactoring (API deprecation)
What is the current behavior?
POST /v1/sources/{source_id}/publications/{publication_name} performs a full replacement of the publication's table list, but using POST is semantically misleading — it implies an additive operation rather than a replacement.
Related issue: #459
What is the new behavior?
Marked the endpoint as deprecated in the OpenAPI summary and description
Added
Deprecation: unix time(RFC 9745) andLink: <...>; rel="deprecation"(RFC 8288) response headersA proper replacement endpoint is being designed. See #459.
Additional context
How to test
BEFORE
AFTER