Repository navigation
Strict replication parity: review items 10, 12, 14 and the ClickHouse-only column scope (follow-up to #1437) - #1559
Merged
Merged
Conversation
…rect-override mismatch (Spec 08.05 s3.3.2)
ColumnTypeOverrideReconciler#reconcileAliasOverrides auto-applies a missing
or drifted column_type_override.alias.* entry via ALTER TABLE ... ADD/MODIFY
COLUMN ... ALIAS. Three ways this could fail left the table silently out of
sync with the declared override, unlike a direct override, which already
halts via ColumnTypeOverrideMismatchException:
- a rejected ADD/MODIFY COLUMN ... ALIAS statement was only logged, and
the drift was never retried;
- an unreadable system.columns silently skipped both the direct and the
ALIAS checks for the whole table;
- the alias column's configured name already belonging to a non-ALIAS
column produced no DDL and no log line at all.
All three now throw ColumnTypeOverrideMismatchException (reused, not a new
exception type; a new (String, Throwable) constructor preserves the
underlying SQLException as the cause). It carries no ClickHouse error code
and is not a terminal type for ClickHouseErrorClassifier, so it classifies
UNKNOWN and ClickHouseBatchRunnable retries the same batch with exponential
backoff -- the same mechanism a direct-override mismatch already relies on,
now applied symmetrically, satisfying Invariant I15 (bounded, declared
recovery) and I9 (no silent continuation past a condition that leaves
ClickHouse different from the source).
Per a scope ruling from the coordinator: parity is required only for columns
that match the source, not for columns a user added to ClickHouse by hand.
Both override kinds (this fix touches only the ALIAS path; the direct path
already had the equivalent check) are skipped silently -- no DDL, no halt --
when the override's source column is absent from the table being
reconciled. This keeps a wildcard override that legitimately does not apply
to a given table from halting it, and guarantees a ClickHouse-only column is
never inspected or altered, even one that happens to carry the exact name a
generated ALIAS column would have.
reconcile()/getExistingColumns()/reconcileAliasOverrides() no longer declare
a checked exception (every failure path in this class now throws the
unchecked ColumnTypeOverrideMismatchException), so ClickHouseAutoCreateTable
createNewTable()'s try/catch around the reconcile() call -- which rethrew
the mismatch exception but silently swallowed anything else via
catch (Exception e) { log.error(...) } -- is removed; there is nothing left
to catch, and removing it also closes that latent catch-all for any other
exception the reconciler could throw.
Spec: 08-schema-catalog/05-auto-table-creation.md section 3.3 gains a
cross-reference, new section 3.3.2 documents the halt and the scope guard,
and new FM-08.05-5 (Failure Modes & Recovery) is added and marked FIXED.
Verification Criteria lists the new/changed tests.
Tests (ColumnTypeOverrideReconcilerTest, all new except the first):
- addColumnDdlFailureHalts(), modifyColumnDdlFailureHalts(),
nonAliasColumnCollisionHalts(), existingColumnsReadFailureHalts() pin
the four halt triggers.
- wildcardOverrideForAbsentSourceColumnIsIgnored(),
clickHouseOnlyAliasColumnForAbsentSourceColumnIsNeverTouched() pin the
source-matching scope guard from the coordinator's ruling.
- addColumnDdlSucceedsForSourceMatchingColumn() confirms the existing
auto-add behaviour for a source-matching column is unaffected.
No existing test encoded the old log-and-continue behaviour, so none needed
to be rewritten; columnMetadataQueryBindsTheNames() (section 3.3.1,
pre-existing) is unchanged and still passes.
Mutation-checked: reverted the three main-source files (the test file and
spec left in place) and re-ran the suite -- the build failed to compile
(three of the new test methods call reconcile() directly, and the pre-fix
signature still declares "throws Exception"), confirming the new tests
cannot pass without the fix. Restored the fix; recompiled clean.
Verification (boxcar sandbox, docker.io/jenkins/jenkins:lts-jdk17, mvnw -o):
mvn test -Dtest=ColumnTypeOverrideReconcilerTest
Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
mvn test -Dtest=ClickHouseAuto
F438
CreateTableTest,ColumnTypeOverrideReconcilerTest
Tests run: 9, Failures: 0, Errors: 0, Skipped: 0 -- in ClickHouseAutoCreateTableTest
Tests run: 8, Failures: 0, Errors: 0, Skipped: 0 -- in ColumnTypeOverrideReconcilerTest
python3 scripts/validate_specs.py -- Validation PASSED (83 specifications,
1215 verification citations checked).
Destructive-Op-Check: sites=1; result=pass
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ntinuing (spec 10.04 s3.9) Defect: a PostgreSQL source column that could not be confirmed present on ClickHouse was a silent divergence, not a loud failure. Three places caught and logged past exactly this condition instead of halting, breaking parity with the MySQL path (whose SQL thread stops and does not advance its position on an error): - `ClickHouseConverter.extractDebeziumSchema` caught any exception while reading a record's own schema and returned `null` after a WARN, so the caller treated "I could not tell what columns this row has" the same as a legitimate no-row record. - `PostgresSchemaChangeDetector.checkAndReconcile` wrapped its whole body in a catch-all that logged and returned for a null writer/connection, a failed `system.columns` fetch, and a failed `addMissingColumns` call. Worse, because `lastReconcileAttempt` was recorded unconditionally before the reconciliation call, a failure was also masked for the following 10s cooldown window: every record in that window passed through unchecked. - `PostgresSchemaReconciler.addMissingColumns` logged each failed `ALTER TABLE ... ADD COLUMN` and returned normally, so the caller believed every missing column had been added. - The call site in `DebeziumChangeEventCapture#processEveryChangeRecord` caught whatever the detector threw and logged "Schema drift detection threw unexpectedly; continuing replication". Fix: let all of the above propagate. `addMissingColumns` still attempts every missing column (one bad column must not block the others) but now throws afterward naming every column that failed, with the first rejection as cause. `checkAndReconcile` no longer catches; it still returns normally only for the two legitimate non-failure outcomes (a record extraction that correctly returns null, and a table confirmed absent from ClickHouse, which the connector is expected to auto-create). It now also tracks the last reconciliation failure per table so a cooldown-window check re-throws the stored failure instead of passing records through silently. The `processEveryChangeRecord` call site wraps whatever `checkAndReconcile` throws in `RecordReplicationException` -- the same exception type and wrap-and-rethrow shape already used a few lines below for a parser failure -- so it escapes the generic catch-all and halts the engine the same way an unconvertible row does: offset not committed, retry/terminal-exit policy applies on restart. Parity scope (ruling applied here and stated in spec 10.04 s3.9): parity is required only for columns that exist in the source and for tables the connector manages. A column a user has added to a ClickHouse table that does not exist in the source is never flagged, never reconciled, and never halts the pipeline -- `findMissingColumns` inspects only the Debezium-side field list, by construction. Spec: specs/10-resilience-monitoring/04-loud-failure-guarantee.md section 3.9 rewritten (previously described the WARN-and-skip behaviour as correct); FM-10.04-8 failure mode entry added; Verification Criteria updated to the five tests below. Tests (all new or changed to assert the halt instead of the skip): - `ClickHouseConverterSchemaExtractionTest.unreadableSchemaPropagates()` (rewritten from `unreadableSchemaIsLoggedAtWarn`, which asserted `assertDoesNotThrow`); `nullValueSchemaReturnsNullWithoutThrowing()` pins the still-legitimate null case. - `PostgresSchemaChangeDetectorTest`: `genuineRowWithUnreachableWriterThrows()` and `cdcInternalColumnsEnvelopeStillThrows()` (rewritten from two tests that asserted `assertDoesNotThrow` for the same inputs); `cooldownAfterAFailureStillThrows()` (new, pins the cooldown-masking fix via reflection-prepopulated cache state); `extraClickHouseOnlyColumnNeverHalts()` (new, pins the parity-scope ruling: a ClickHouse-only column never halts even with no writer wired). - `PostgresSchemaReconcilerAddColumnFailureTest.failedAlterThrowsAfterAttemptingAllColumns()` (new): a rejecting JDBC connection proxy confirms all 3 missing columns are attempted and the thrown exception names all 3. - `SchemaDriftFailureIsTerminalTest.schemaDriftFailurePropagatesThroughProcessEveryChangeRecord()` (new): exercises the real `processEveryChangeRecord` seam with a rejecting-connection-backed detector wired through `PostgresConnectorConfig`; confirms `RecordReplicationException` naming "schema-drift" escapes. Mutation-check: each of the four production edits (ClickHouseConverter.java, PostgresSchemaChangeDetector.java, PostgresSchemaReconciler.java, DebeziumChangeEventCapture.java) was individually reverted in isolation (git stash of that one file) and the corresponding test(s) re-run in an isolated container sandbox; every one went red with the expected failure/assertion message, then the file was restored and the suite re-confirmed green. Verification (isolated container sandbox, offline `-o` Maven reactor build `-pl sink-connector,sink-connector-lightweight -am`): Tests run: 22, Failures: 0, Errors: 0, Skipped: 0 (ClickHouseConverterSchemaExtractionTest: 2; PostgresSchemaChangeDetectorTest: 18; PostgresSchemaReconcilerAddColumnFailureTest: 1; SchemaDriftFailureIsTerminalTest: 1) BUILD SUCCESS. `python3 scripts/validate_specs.py`: Validation PASSED (83 specifications, 1217 verification citations checked). Not touched: PreparedStatementExecutor, ColumnTypeOverrideReconciler, and the alias-override code in PostgresSchemaReconciler (ensureAliasColumns/ensureAllAliasColumns/fetchColumnNames) -- owned by a different work item. Docker-dependent integration tests were not run (no Docker in the sandbox); this change only touches unit-testable code paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…knowledged, not the whole record list (Spec 03.06 section 3.5) A chunked batch's failure was retried from scratch: when chunk k of a BatchChunker split threw, the worker retried the whole batch (spec 03.03 FM-03.03-2) with the same retained record list, and nothing distinguished a record whose write ClickHouse had already acknowledged from one it had not. Every retry therefore resent chunks 1..k-1 again, resent a flushStagedRows() flush that had already landed, re-ran an inline SCD2 UPDATE/DELETE history statement that had already run, and re-ran a TRUNCATE/bulk-close that had already applied. Destructive-Op-Check: sites=4; result=pass
…stead of warning and failing (Spec CONSTITUTION I6 parity scope, Spec 11.02 sections 3.3, 3.9, FM-11.02-9, FM-11.02-10)
A later ruling narrows what the replication contract covers: parity is
required only for columns that exist on the MySQL source (or that the
connector itself creates and manages for replication bookkeeping), not for
every column a ClickHouse table happens to carry. A column added on the
ClickHouse replica alone, with no column of that name on the source table,
has no source value to conform to or diverge from, so it is tolerated. This
reverses the previous default, which named such a column at WARNING, failed
the scheduled job's "no WARNING in the output" contract, and exited 1 unless
--allow_replica_only_columns was passed.
- specs/CONSTITUTION.md: new "Parity scope" paragraph under Invariant I6,
the single place this rule is stated; it does not relax Invariant I9 (an
unapplied or lost source change is still a loud halt) because a
ClickHouse-only column has no source value to apply or lose.
- clickhouse_table_checksum.py restrict_to_source_columns(): the replica-only
branch now logs once per table at INFO ("... tolerated, not compared)")
instead of WARNING. The opposite case -- a source column the replica lacks
-- is unchanged: still a WARNING, still a real difference, since parity is
in scope for it.
- top_level_table_checksum.py: record_replica_only() always logs the
tolerated finding at INFO (no more flag-gated WARNING branch);
relay_side_messages() now checks record_replica_only() ahead of, and
independently of, the generic WARNING-marker branch, so the report is
recognised regardless of which level the side emits it at;
report_run_summary()'s end-of-run line is INFO and no longer sets exit
code 1 for these findings. --allow_replica_only_columns stays an accepted
flag (Invariant I11: no flag is removed) but is now a no-op; its --help
text says so.
Tests: renamed/rewrote the three tests that pinned the old WARNING/exit-1
default to pin the new tolerated/INFO/exit-0 default instead --
TestReplicaOnlyColumns (test_checksum_fidelity.py),
TestSourceColumnSetReachesTheReplicaSide (test_checksum_verdicts.py),
TestScheduledJobLogContract (test_checksum_job_log_contract.py). The
"source column missing on the replica" tests in the same classes, which
exercise the still-in-scope case, are unchanged. Mutation-checked: reverting
just the two source files (git stash) turns 6 tests red --
test_replica_only_column_is_left_out_and_named_once_at_info,
test_a_name_with_a_comma_or_a_space_survives_the_driver_to_side_trip,
test_by_default_a_replica_only_column_is_tolerated_at_info,
test_the_allow_flag_is_a_no_op,
test_replica_only_column_is_tolerated_by_default,
test_allow_replica_only_columns_flag_is_a_no_op; restoring the fix turns
them green again.
Verification: python3 -m pytest -q db_compare/tests db_load/tests
db_dump/tests tests: 805 passed, 2 skipped, 30 subtests passed (unchanged
from the pre-change baseline -- the renamed tests replace the old ones
one-for-one, no existing test weakened or dropped). scripts/validate_specs.py:
PASSED (83 specs, 1212 citations).
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…pture into VersionSequencer (Spec 02.01 section 2, Spec 02.02 section 2, Spec 02.03 section 2) DebeziumChangeEventCapture.java had grown to ~4,100 lines even after commit 19c1bea extracted DdlTableNames. The `_version` sequencer -- the `effectiveTs * 1_000_000 + counter` formula, its process-wide floor, its intra-window counter, its source-log high-water mark, and the restart-floor seeding -- was ~300 lines of self-contained, stateless-logic-over-shared- statics with no dependency on the rest of the engine object. It moves unchanged to the package-private VersionSequencer, next to its user: - The 7 static fields (SEQUENCE_START, SEQUENCE_START_INITIAL, sequenceNumber, sequenceAnchorTs, sequenceHighWaterPosition, sequenceHighWaterEffectiveTs, sequenceMaxSourceTs) and their javadoc, verbatim. - nextSequenceNumber, the VersionAssignment inner class, nextVersionAssignment, seedVersionFloor, raiseVersionFloor and addVersion, verbatim. No logic changes, no renamed config keys, same log text, same exceptions. addVersion's visibility is narrowed from public to package-private: it has no caller outside the cdc package (confirmed by grep across both modules), matching the all-package-private convention of DdlTableNames. seedVersionFloorFromDurableMark and handleChangeEventBatch stay on DebeziumChangeEventCapture (they are entangled with the engine's own Properties/config lifecycle) and now call into VersionSequencer. The statics remain process-wide mutable state shared across engine restarts within one JVM -- this extraction only moves which class declares them, it does not change their lifetime or visibility to the rest of the process (still package-private to `cdc` via the class, but the fields themselves stay `public static` so the existing test hooks that reset them via direct field access keep working unchanged, just via a new qualifier). Formal model: formal_specs/lean/Replication/VersionFloor.lean names `DebeziumChangeEventCapture.nextSequenceNumber` / `DebeziumChangeEventCapture.seedVersionFloor` in two doc comments; both now read `VersionSequencer.*`. The Lean state machine itself is unchanged. Specs updated to point Codebase Mapping / qualified references at the new file (Primary Source and/or inline qualified mentions only -- specs that mention the moved methods/fields in unqualified prose are left as-is, per the precedent set by 19c1bea's own spec diff): - specs/02-versioning/01-version-formula-encoding.md - specs/02-versioning/02-commit-monotonicity-floor.md - specs/02-versioning/03-sequence-counter-lifecycle.md - specs/02-versioning/04-redelivery-stability.md - specs/02-versioning/06-upgrade-compatibility.md - specs/01-cdc-engine/04-transaction-boundaries.md - specs/01-cdc-engine/02-binlog-coordinates.md - specs/01-cdc-engine/08-compressed-transaction-payloads.md - specs/09-offset-management/03-clickhouse-offset-store.md Tests: the 17 test files that reset the statics or construct/invoke VersionAssignment / nextSequenceNumber / nextVersionAssignment / seedVersionFloor / raiseVersionFloor / addVersion via reflection or direct package-private access are updated to the DebeziumChangeEventCapture.X -> VersionSequencer.X qualifier; no assertion changed. No new regression test is added -- this is a pure move, and the existing suite already pins every behaviour of the sequencer; mutation check is therefore N/A in the usual revert-the-fix sense (there is no fix, only a move), and is instead a move check: `git diff` shows the moved bodies are byte-identical to the pre-move text modulo the class qualifier at call sites, and the test run below is unchanged in count from the pre-move baseline. Verification: - `python3 scripts/validate_specs.py` -> PASSED (83 specifications, 1212 verification citations). - `./mvnw -pl sink-connector-lightweight test`: before and after this change, Tests run: 784, Failures: 0, Errors: 76, Skipped: 28 -- identical counts; all 76 errors are the known Docker-environment baseline (testcontainers *IT classes cannot reach a Docker daemon in this environment). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…en (Spec 04.03 FM-04.03-4, Spec 11.02 FM-04.03-4 Blast radius) A later ruling narrows what the replication contract covers (Constitution I6, parity scope): a column that exists on the ClickHouse replica alone, with no column of that name on the MySQL source and not created or managed by the connector for replication bookkeeping, is tolerated rather than a replication gap. The Python checksum side of this ruling was already fixed in a prior commit on this branch; this commit covers the Java connector's own write path, per the follow-up audit: does the connector ever try to supply a value for such a column, warn about it, or ALTER it? Investigation (QueryFormatter.createColumns, PreparedStatementFieldMapper. insertPreparedStatement, GroupInsertQueryWithBatchRecords, DbWriter, DBMetadata.getColumnsDataTypesForTable) found the connector already does the right thing, by the same mechanism the #1389 pre-ALTER fix put in place: - A ClickHouse-only column can never be a member of any record's schema (no source column of that name ever produces one), so QueryFormatter.createColumns() never gives it a placeholde 8000 r in the generated INSERT -- in every batch, not just the first one after an ADD COLUMN, because there is no ADD COLUMN on the source side to catch up to. - At bind time, PreparedStatementFieldMapper.insertPreparedStatement finds no entry for it in columnNameToIndexMap, finds it is not isUnboundByDesign, finds recordCarries(fields, colName) false, and takes the existing by-design branch: DEBUG log, continue. No value is bound, no exception is thrown, no ALTER is issued; ClickHouse keeps whatever value the column already holds. This is the #1389 mechanism's Case B, applied to a column that is absent *permanently* (no source counterpart will ever arrive) rather than *transiently* (pending an ALTER) -- mechanically identical, different rationale, so it gets its own named failure mode and its own test rather than extending the pre-ALTER one. No production code changes: this is a "pin existing-correct-behaviour" change (outcome (a) of the audit), not a bug fix. - specs/04-query-generation/03-field-membership-rules.md: added a paragraph to section 3.1 distinguishing Case B's transient (pre-ALTER) and permanent (replica-only) triggers; added FM-04.03-4 ("A column only ClickHouse has"); added the two new tests to section 5 Verification Criteria; updated the failure-mode count (3 -> 4). - specs/11-verification-tooling/02-db-compare-checksums.md: corrected FM-11.02-10's "Blast radius" sentence, which previously only pointed at spec 04.03 without saying what it found there, to state plainly that the connector's write path already excludes such a column from every generated INSERT, so nothing worse than an unjudgeable column happens. - sink-connector/src/test/java/.../db/batch/ClickHouseOnlyColumnTest.java (new): two tests through the two real entry points -- testClickHouseOnlyColumnIsOmittedFromGeneratedInsert (query-formatting, through GroupInsertQueryWithBatchRecords.updateQueryToRecordsMap) and testClickHouseOnlyColumnRecordWritesWithoutError (bind-time, through PreparedStatementFieldMapper.insertPreparedStatement, using a JDK dynamic proxy PreparedStatement stand-in -- this module has no mocking framework on its test classpath, following the pattern already used by NullValueColumnDropTest). The proxy records every touched index from any setXxx(int, ...) call rather than naming individual JDBC setter methods: a first version that matched only setNull/setString/setInt/setLong missed that an Int32 column binds via setObject (ClickHouseDataTypeMapper's isWiderIntegerTarget treats Int32 itself as a possible post-ALTER widening target, so it is not routed through setInt), which silently dropped the "id" column's own bind from the recorded indices and failed the test for a reason that had nothing to do with the behaviour under test. Mutation-checked: since no production code changed, the check instead confirms the two new tests actually exercise the mechanism being pinned. Temporarily disabled, in the two production methods the javadoc names, only the specific condition this change pins (QueryFormatter.createColumns's record-field-membership check at the "omitted from INSERT" branch; and PreparedStatementFieldMapper.insertPreparedStatement's `recordCarries` branch), confirmed both new tests red: ClickHouseOnlyColumnTest.testClickHouseOnlyColumnIsOmittedFromGeneratedInsert: AssertionError -- ch_added_col got a placeholder in the generated INSERT. ClickHouseOnlyColumnTest.testClickHouseOnlyColumnRecordWritesWithoutError: StaleSchemaCacheException -- "Column ch_added_col is present in the ClickHouse table and carried by the record, but has no placeholder in the generated INSERT ... Failing the batch instead." Restored both methods; both tests green again. No other test's behaviour depends on this change, so no test was weakened, deleted, or had its assertions loosened. Verification (isolated container, JDK 17, offline Maven; Docker-backed testcontainers/*IT tests cannot run in this environment, the known baseline): ./mvnw -o -pl sink-connector -am -Dtest=ClickHouseOnlyColumnTest, NullValueColumnDropTest,QueryFormatterPreAlterRecordTest, PreparedStatementFieldMapperRecordCarriesTest, PreparedStatementFieldMapperUnboundColumnTest test Tests run: 23, Failures: 0, Errors: 0, Skipped: 0 (2 new + 21 pre-existing, unchanged). ./mvnw -o -pl sink-connector -am test (full module) Tests run: 770, Failures: 0, Errors: 20, Skipped: 41 -- all 20 errors are DBMetadataTest/DbKafkaOffsetWriterTest/DbWriterTest/ ClickHouseCreateDatabaseTest failing with "IllegalState: Previous attempts to find a Docker environment failed", the pre-existing testcontainers/ Docker-unavailable baseline for this environment, unrelated to this change. python3 scripts/validate_specs.py: Validation PASSED (83 specifications, 1214 verification citations). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Runtime mutation check for commit d4310eb (item 12, ALIAS override reconciliation). The commit message reports a compile-time check only; this is the runtime check, recorded here because rewriting the commit would need a force-push. Each fix was disabled one at a time in ColumnTypeOverrideReconciler (method signatures unchanged) and ColumnTypeOverrideReconcilerTest was run:
|
…quencer (fixes testCompile on the combined head) SchemaDriftFailureIsTerminalTest was written against DebeziumChangeEventCapture.VersionAssignment; the version-sequencer extraction (4f3f809) moved the nested class to VersionSequencer, and the two commits were combined without a compile in between. No behaviour change. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…being swallowed (Spec 10.04 section 3.10, FM-10.04-9; precedent Spec 06.03 FM-06.03-1) The PostgreSQL DDL parse path (PostgreSQLDDLParserService#parseSql and all five enter*() callbacks in PostgreSQLDDLParserListenerImpl) wrapped translation in try/catch and logged-and-continued on any error, including statements the grammar could not parse. MySQL has no such catch: a syntax error propagates via ErrorListenerImpl#syntaxError (a RuntimeException), is wrapped in DDLReplicationException, and halts replication with retry/backoff instead of advancing the offset past an untranslated change (Spec 06.03 FM-06.03-1). Fix: reuse MySQL's own ErrorListenerImpl unchanged, registered on the PostgreSQL lexer and parser in a new PostgreSQLDDLParserService#runAntlrPipeline helper, and remove every try/catch from the five enter*() methods in PostgreSQLDDLParserListenerImpl (enterCreatestmt, enterAltertablestmt, enterRenamestmt, enterDropstmt, enterTruncatestmt). The bespoke LenientErrorListener that only logged is deleted; no second halting mechanism was invented, reusing MySQL's exact type, listener, and propagation path. Five things remain intentionally skipped without throwing (spec 10.04 section 3.10's allowed-skip list, each a dedicated early-return or reporting call decided before translation, never a caught exception): 1. Statement kinds with no listener callback (CREATE INDEX, CREATE SEQUENCE, CREATE VIEW, COMMENT ON, ...) parse fine and produce an empty translation plus a WARN log, mirroring MySQL's own FM-06.03-3. 2. DROP of a non-TABLE object (early return in enterDropstmt). 3. An unrecognized alter_table_cmd keyword (default branch in translateAlterTableCmd, DEBUG log). 4. An untranslatable constraint clause, reported via reportUntranslatedConstraint (Spec 06.09 section 3.7), unchanged by this fix. 5. ClickHouse-only tables/columns, tolerated by design under Parity scope (Spec 10.04 section 3.9). Spec: updated specs/10-resilience-monitoring/04-loud-failure-guarantee.md - Codebase Mapping cites the PostgreSQL parse path and its reuse of ErrorListenerImpl. - New section 3.10 'A PostgreSQL DDL translation failure halts, not merely warns': before/after behavior and the five-item allowed-skip list above. - Verification Criteria cites the two new tests plus the three existing allowed-skip-kind tests and PostgreSQLConstraintClauseReportTest. - New failure mode FM-10.04-9, following the sibling entries' Trigger/Behaviour/Detection/Blast radius/Recovery/RTO/Test structure. - Summary count updated to 9 failure modes. python3 scripts/validate_specs.py passes (83 specs, 1229 verification citations checked). Tests: added unparseablePostgresDdlPropagatesInsteadOfBeingSwallowed and unparseablePostgresDdlPropagatesThroughAtomicBooleanOverload to PostgreSQLDDLParserServiceTest; both feed an unparseable statement to the two parseSql overloads and assert a RuntimeException containing 'Error parsing DDL' escapes with nothing appended to the output buffer. Negative proof (reverted both source files to HEAD via git stash, reran the class in an isolated offline test environment, then restored the fix): pre-fix: Tests run: 38, Failures: 2, Errors: 0 -- both new tests failed: Expected RuntimeException to be thrown, but nothing was thrown. post-fix: both tests pass; log shows 'ERROR - Error parsing' for each unparseable statement; the other 36 tests in the class unaffected. Full build (build.sh, RUN_TESTS=1, isolated offline test environment, sink-connector installed -DskipTests then both modules tested) versus the e9068e8 baseline: sink-connector-lightweight: tests=790 (788+2 new) failures=0 errors=76 skipped=28 sink-connector: tests=780 failures=0 errors=20 skipped=41 Error counts match baseline; every failing/erroring testcase (verified via surefire XML) is a Testcontainers/Docker *IT class or container-startup failure, expected offline, not a regression. No existing test was weakened, skipped, or deleted. Destructive-Op-Check: sites=4; result=pass Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…umChangeEventCapture into SystemDbConnectionRetry (Spec 01.01 section 3.5) DebeziumChangeEventCapture.java was still ~3,800 lines after commits 19c1bea (DdlTableNames) and 4f3f809 (VersionSequencer). The initial system/offset database connection retry -- SYSTEM_DB_CONNECT_ATTEMPTS, SYSTEM_DB_CONNECT_RETRY_MS, createSystemDbConnectionWithRetry and connectWithRetry -- was ~100 lines of self-contained, stateless retry logic with no dependency on the rest of the engine object beyond the credentials/config it is handed. It moves unchanged to the package-private SystemDbConnectionRetry, next to its caller: - SYSTEM_DB_CONNECT_ATTEMPTS and SYSTEM_DB_CONNECT_RETRY_MS, and their javadoc, verbatim. - createSystemDbConnectionWithRetry and connectWithRetry, verbatim, including log levels/messages. setSystemDbConnection on DebeziumChangeEventCapture now delegates to SystemDbConnectionRetry.createSystemDbConnectionWithRetry instead of calling the method on itself; no other behavior changes. SystemDbConnectRetryTest moves its reflection/reference target from DebeziumChangeEventCapture to SystemDbConnectionRetry (qualifier only; no assertion changed). Spec 01.01 (embedded engine lifecycle) gets a new Codebase Mapping bullet, a new section 3.5 describing the retry behavior, and the existing SystemDbConnectRetryTest citation is repointed at the new class. python3 scripts/validate_specs.py passes (83 specs, 1227 verification citations). Pure move: both the production code and the test were otherwise byte-identical before and after save for the DebeziumChangeEventCapture -> SystemDbConnectionRetry qualifier swap; no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…SqlDDLParserListenerImpl into DefaultLiteralTranslator (Spec 06.04 section 3.2.1, 3.2.2 rules 1-2) MySqlDDLParserListenerImpl.java was still 3,226 lines after commits 19c1bea (DdlTableNames) and 4f3f809 (VersionSequencer). The literal DEFAULT rewrite -- literalDefault, bytesLiteral, clickHouseString, stringBody, isCurrentTimestampDefault and firstEnumMember, plus the NUMERIC_CH_TYPE pattern they share -- is ~160 lines of stateless, side-effect-free parse-tree logic with no dependency on the listener's own state (config, database/table names, the DDL event timestamp, the logger). It moves unchanged to the package-private DefaultLiteralTranslator, next to its caller: - NUMERIC_CH_TYPE and its javadoc, verbatim. - literalDefault, bytesLiteral, clickHouseString, stringBody, isCurrentTimestampDefault and firstEnumMember, verbatim, including their javadoc. literalDefault, isCurrentTimestampDefault and firstEnumMember become package-private statics (resolveDefault calls them); bytesLiteral, clickHouseString and stringBody stay private, since only the moved methods call them. resolveDefault on MySqlDDLParserListenerImpl now delegates to DefaultLiteralTranslator.literalDefault/isCurrentTimestampDefault/ firstEnumMember instead of calling the methods on itself; no other behavior changes. eventInstantLiteral and backfillNotReproducible stay behind -- they need the listener's own ddlEventTimestampMs/ databaseName/cleanTableName/originalSql/log -- along with the DATETIME64_SCALE/UTC_SECONDS constants eventInstantLiteral uses. The no-longer-used java.math.BigInteger import is dropped from the old file; DefaultLiteralTranslator has no logger, since none of the moved code logs (matching the DdlTableNames precedent). None of literalDefault/bytesLiteral/clickHouseString/stringBody/ isCurrentTimestampDefault/firstEnumMember/NUMERIC_CH_TYPE is referenced by any test, directly or via reflection: every existing DEFAULT test in MySqlDDLParserListenerImplTest exercises this logic through the public translate(...) entry point, so no test file changes are needed. Spec 06.04 (ALTER TABLE clause translation) gets a new Codebase Mapping bullet naming DefaultLiteralTranslator and the methods/sections it implements, and resolveDefault is added to the Primary Source bullet's method list. python3 scripts/validate_specs.py passes (83 specs, 1227 verification citations). Pure move: the production code is byte-identical before and after save for the MySqlDDLParserListenerImpl -> DefaultLiteralTranslator qualifier swap at the three call sites in resolveDefault; no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…cy out of MySqlDDLParserListenerImpl into ReplacingMergeTreeKeyPolicy (Spec 06.05 section 3.6) MySqlDDLParserListenerImpl.enterColumnCreateTable carried the full sorting-key derivation for a CREATE TABLE -- declared PRIMARY KEY, then a NOT NULL UNIQUE key, then every stored column as a last resort, with the allow_nullable_key determination, the is_deleted/_is_deleted collision check, the ReplacingMergeTree/ReplicatedReplacingMergeTree engine-clause selection, and the ORDER BY index-prefix clean-up -- all inline in one already-large method. This is stateless, logic determining the table's identity and engine from the source key, not column/type emission, so it moves to its own package-private class, next to its caller: - resolveSortingKey: the PRIMARY KEY / UNIQUE-key-fallback / all-columns -fallback derivation (moved verbatim, including every rationale comment and the INFO/WARN/ERROR logging and the DDLReplicationException refusal for a table with no stored column), now taking orderByColumns/uniqueKeyColumns/orderedColumnNames/notNullColumnNames/ databaseName/tableName/overridePrimaryKey/originalSql as parameters and returning the nullableSortingKey flag. - resolveIsDeletedColumnName: the is_deleted/_is_deleted disambiguation loop, moved verbatim. - engineClause: the four-way ENGINE=... text selection, moved and restructured into one method returning the clause text (leading space preserved) for the caller to append. - sanitizeOrderBy: the id_registro(10)-style suffix clean-up regex, moved verbatim into a function returning the sanitized string. - splitIndexColumns: moved in full (not just visibility-widened), since grep confirms its only call site is inside the sorting-key derivation that is itself moving; its one internal call becomes MySqlDDLParserListenerImpl.stripBackticks(part). stripBackticks loses its `private` modifier (package-private now) so the new class can call it across the package boundary; no other change to it. The now-unused KeylessTableWarning import is dropped from the old file (the one call that used it moved to the new class). The schema-override primary_key/overridePrimaryKey lookup stays in enterColumnCreateTable -- it is passed into resolveSortingKey as a parameter -- along with the column/engine text emission, the PARTITION BY/TTL/SETTINGS emission and the appendOrderBy calls themselves, since all of those need the listener's own query/config state. parseCreateTable/parseColumnDefinitions are untouched: they are general column/type emission entangled with the listener's query buffer, not part of the key/engine policy. None of the moved methods is referenced by any test, directly or via reflection: every existing sorting-key/engine test in MySqlDDLParserListenerImplTest (including testCreateTableKeylessOrdersByAllColumns) exercises this logic through the public translate(...)/listener entry point, so no test file changes are needed. Spec 06.05 gets a new Codebase Mapping bullet naming ReplacingMergeTreeKeyPolicy and the methods/sections it implements. python3 scripts/validate_specs.py passes (83 specs, 1227 verification citations). Pure move: production code is byte-identical before and after for the MySqlDDLParserListenerImpl -> ReplacingMergeTreeKeyPolicy qualifier swap at the four call sites in enterColumnCreateTable; no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
minguyen9988
marked this pull request as ready for review
October 7, 2026 15:50
…offset watermark (Spec 03.06 section 3.5) Section 3.5 says the watermark used for the offset commit reflects every row that is actually durable, whichever attempt wrote it, and the grouping path honours that by folding an already-applied record's offset into partitionToOffsetMap. The short-circuit in ClickHouseBatchRunnable.processRecordsByTopic (every record of the topic is already marked appliedToClickHouse) returned true before grouping, so it never reached the durablyInsertedOffsets merge. If the attempt that wrote those rows returned false before its own merge, the rows were durable in ClickHouse but outside the committed offset, and a restart would redeliver and re-insert them. Fix: the short-circuit merges each record's offset into durablyInsertedOffsets (max per TopicPartition, the same merge the normal path uses) before returning true. ClickHouseBatchWriter has no durable watermark, so its short-circuit is unchanged. Spec: 03.06 section 3.5 states the fold for the short-circuit; Verification Criteria cites the new tests. Tests: AllAppliedTopicWatermarkTest (no server; connections are null and the short-circuit never uses one). - anAllAppliedTopicAdvancesTheDurableWatermark: without the fix it fails with "partition 0 must advance from 5 to the highest durable offset, 9 ==> expected: <9> but was: <5>"; with the fix it passes. - anAllAppliedTopicNeverMovesTheWatermarkBackwards: a lower offset never lowers the watermark. ClickHouseBatchRunnableTest, WorkerFailureModesTest, ParkedBatchWrittenOnceTest, PreparedStatementExecutorFailureModesTest and PreparedStatementExecutorTruncateTest pass unchanged (their skipped tests are the existing @disabled ones). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rejects [spec-exempt: build metadata only; the resolved dependency set is unchanged]
The CI build broke on the GitHub runner image update from ubuntu24/20260927.320
to ubuntu24/20261004.327, which moves Maven from 3.9.16 to 3.10.0. The first
build step runs `mvn -q ... exec:exec` in sink-connector and writes its output
into GITHUB_ENV; Maven 3.10 now reports duplicate dependency declarations as an
ERROR that -q does not silence, so the step wrote
"[ERROR] 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be
unique: org.apache.kafka:connect-json:jar -> duplicate declaration of version
${version.kafka} @ line 425" into GITHUB_ENV and failed with "Unable to process
file command 'env' successfully". Every job after it was skipped. The same
poms passed on the previous image (run on dcfb9e0: 15 of 15 green).
Duplicates removed (a scan of every pom.xml finds no others):
- sink-connector/pom.xml: org.apache.kafka:connect-json, declared twice, both
compile scope.
- sink-connector-lightweight/pom.xml: org.apache.kafka:connect-json (test, then
compile), org.apache.commons:commons-lang3 3.12.0 (test, then compile),
org.apache.logging.log4j:log4j-api (2.17.1, then 2.23.1 compile).
In each pair the LAST declaration is kept, because that is what Maven 3.9
resolved: the jar built from the unchanged poms shades log4j-api 2.23.1,
commons-lang3 3.12.0 and org/apache/kafka/connect/json, i.e. the compile-scope
and later-version entries. Keeping them leaves the effective dependency set
exactly as it was.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… (Maven 3.10 rejects duplicate plugins) [spec-exempt: build metadata only; the plugin configuration the build uses is unchanged] After the duplicate dependencies were removed, the same runner-image update (Maven 3.9.16 -> 3.10.0) failed the next step that reads sink-connector-lightweight/pom.xml (`mvn versions:set-property`): "'build.plugins.plugin.(groupId:artifactId)' must be unique but found duplicate declaration of plugin org.apache.maven.plugins:maven-compiler-plugin @ line 629". Maven 3.10 promotes duplicate plugin declarations from a warning to an error (release notes: "duplicated plugin entries will not be tolerated anymore"). The pom declared maven-compiler-plugin twice: 3.15.0 with lombok 1.18.34 as the annotation processor, then 3.11.0 with lombok 1.18.30. Maven 3.9 used the last one: the earlier CI log on this branch shows "maven-compiler-plugin:3.11.0:testCompile ... on project clickhouse-debezium-embedded". The 3.11.0 declaration is kept and the unused 3.15.0 one removed, so the compiler and annotation-processor versions the build runs with do not change. A scan of every pom.xml (dependencies, plugins, plugin executions, modules) now finds no duplicate declarations. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…kip, no XFail [spec-exempt: test-suite selection only, no product code] The connector must replicate with source-to-destination parity, so the end-to-end suite has to pass in every test, not only in the ones it chooses to run. Until now the two suites the pull-request pipeline runs (regression_auto.py, regression_auto_replicated.py) disabled a large part of themselves: on the 2.11.0 base and on this branch alike each run reported "91 features (74 ok, 14 skipped, 3 xfail)" (auto) and "89 features (72 ok, 14 skipped, 3 xfail)" (replicated). Disabled until now, now enabled: - ffails Skip entries (both regressions, 33 entries each): the whole insert feature, partitions, schema only, cli, truncate of tables without a primary key, the multiple parallel add/modify/drop column alter, the no-primary-key variants of insert/delete/update, primary keys/no primary key, partition limits. - inline xfail() in modules the pipeline loads: truncate.py complex primary key (2), insert.py DEFAULT-with-NULL (2), INSERT with a SELECT constant / SELECT from a table (2), one million datapoints, the no-primary-key examples of many-partitions-one-part / one-million / parallel (3), parallel_alters.py multiple parallel add and rename column. Several of these reasons describe parity defects (for example "doesn't work without primary key as only last row of insert is replicated"). This commit only stops hiding them; the failures it surfaces are fixed in the commits that follow on this branch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…aim [spec-exempt: end-to-end test harness only, no product code]
With every Skip/XFail removed (previous commit) the pipeline's testflows suites
failed the same set of tests in all four jobs. Each failure is in the test
harness: the statement the test sends is rejected by MySQL itself, or the test
races itself, before the connector is involved. Fixed without weakening any
assertion:
- insert/default with null, default with null and non null, use select
constant as value, use select from table as value: the table's primary key
`id` is INT NOT NULL with no default and the INSERT named only
(col1,col2,col3), so MySQL answered "ERROR 1364 (HY000): Field 'id' doesn't
have a default value" (quoted from the CI log). The INSERT now names id.
- insert/parallel (all four table shapes): the three concurrent inserts used
overlapping ids (1, 1..100, 2..1001), so MySQL rejected the later ones with
duplicate keys and the step retried for its whole 300 s; keyless tables are
a single column `x` but were sent two values per row. The inserts now use
disjoint ranges (1, 2..101, 102..1101) and one value per row for keyless
tables (distinct values, since identical whole rows collapse under FINAL in
a keyless ReplacingMergeTree, spec 08.05).
- truncate/complex primary key (both variants): create_mysql_table turned
primary_key="id,k" into "id,k INT NOT NULL," - a column without a type, and
k twice. It now defines only the key columns that the column list does not
already define.
- truncate/no primary key (both variants): four values were inserted into the
three-column (k, c, pad) table; keyless tables now get three.
- parallel alters/multiple parallel add modify drop column: ADD, MODIFY and
DROP of the same column ran in parallel, so MODIFY/DROP could reach MySQL
before ADD ("Unknown column"). They now run in order per column; columns
still run in parallel with each other.
- schema only: the test inserted a row into a table created after the
connector was already streaming and expected it not to replicate, but
snapshot.mode=no_data only skips the initial snapshot. In the replicated suite
it also wrote schema_only.yml to env/auto/configs, which that suite does not
mount, so the connector never restarted. The scenario now stops the
connector, creates a table that already holds a row, starts the connector in
no_data mode with offset and schema-history tables of its own (so a new
snapshot really happens), checks the table is created with no rows, checks a
row written afterwards is replicated, and restarts the connector with the
suite's configuration, restoring the shared in-memory config.
- cli (replicated suite) and multiple databases (replicated suite): cascades
of the dead connector left by schema only. The client calls now assert exit
code 0, so an unreachable connector fails at the call instead of as a later
timeout.
CI: a failing testflows job now prints every failing test's steps and
messages (tfs transform nice-fails on the raw log); --output new-fails printed
details only for the first failure and a bare "AssertionError" for the rest.
Jinja-Render-Check: rendered=1; formats=yaml; result=pass
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ma only [spec-exempt: end-to-end test harness only, no product code] The previous commit fixed truncate, parallel alters, cli and multiple databases: they pass in all four testflows jobs of run 37722722453. Three harness defects remained, now visible thanks to the per-failure log step: - insert/default with null, default with null and non null, use select constant as value, use select from table as value: the rows now reach ClickHouse, but select() compared the test's comma-separated expectation (e.g. "\N,5,333") with `SELECT ... FINAL` in ClickHouse's default tab-separated format, so it could never match and timed out after 300 s. When a test supplies its expected output, the FINAL query now asks for FORMAT CSV, as the two other branches of select() already do. Comparisons against the MySQL client's own (tab-separated) output are unchanged. - insert/parallel, keyless shapes: create_mysql_to_clickhouse_replicated_table gives every shape an `id` column, so keyless tables are (id, x), not (x); the one-value rows from the previous commit were rejected by MySQL. All shapes are back to two values per row, still with disjoint id ranges. - schema only: errored at once with "'TestStep' object has no attribute 'deepcopy'" because the star imports bind a test step named `copy` over the standard-library module. deepcopy is now imported after them under its own name. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Additional end-to-end verification of head ab0733c (connector jar built from this head; Java code identical to 73c44dd), beyond the CI pipeline. Each suite ran in a fresh isolated container: MySQL 8.0 (GTID on unless noted, binlog_transaction_compression on, sql_generate_invisible_primary_key=1), ClickHouse 24.8, the embedded lightweight connector. All checks are value-level (CRC32 over canonical column renderings, MySQL vs ClickHouse FINAL) or structural.
🤖 Generated with Claude Code |
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.
Follow-up to #1437 review items 10-14, under one rule: the connector behaves strictly as a replication program. A condition that would leave a source-matching column or a connector-managed table different from the source halts replication loudly, the offset never advances past an unapplied change, and a retry re-applies only what ClickHouse has not acknowledged. Columns that exist only on the ClickHouse side are tolerated (parity scope, stated in CONSTITUTION.md).
All five review items are in this PR. Commits (spec first, each fix with a test that fails without it):
Behaviour change to note: a PostgreSQL DDL statement the grammar used to recover from partially now halts replication (commit 8). Remaining known residual, stated in Spec 03.06 section 3.5: the one chunk in flight when a failure surfaces may already be applied and is resent on retry.
Whole-Change-Verification: ran=scripts/validate_specs.py (PASSED, 83 specs, 1232 citations) and both Java unit suites offline (sink-connector-lightweight 790 tests, 0 failures; sink-connector 782 tests, 0 failures; the only errors are Testcontainers tests that need Docker, unchanged from the base); a scan of every pom.xml finds no duplicate dependency, plugin, execution or module; CI build job passes on the current head; result=pass