Repository navigation
fix(offsets): coalesced writes report every group before an ack can throw; routing and error-category fixes (#1437 review) - #1566
Merged
Conversation
…y acknowledgement can throw; routing and error-category fixes (PR #1437 review; Spec 09.01 section 3.2, 03.07, 10.01) High: processBatch reported the groups of a coalesced write with one checkIfBatchCanBeCommitted call per group. That call drains the FIFO, and the drain can throw (Debezium markBatchFinished timeout, FIFO consistency checks). A throw on group 1 left groups 2..N written to ClickHouse but never recorded, so their units kept remainingGroups above zero, hasUnwrittenBatches() stayed true, heartbeats stopped committing and the engine eventually stopped at handoff.wait.timeout.ms, replaying everything since the frozen offset. DebeziumOffsetManagement.reportWritten now marks every group first (a per-group mark failure is remembered, never stops the loop), drains once, and rethrows the first failure. Tests: CoalescedQueuedBatchesTest committerFailureOnFirstOfTwoCoalescedGroupsDoesNotOrphanTheSecond and groupThatFailsToMarkDoesNotOrphanSiblingGroups (fails without the fix: the sibling is never acknowledged). Low 1: a failure to report written rows is wrapped in OffsetAcknowledgementException and logged as an offset acknowledgement error in run(), instead of Retriable ClickHouse error (Code: -1, Category: UNKNOWN). Retry behaviour is unchanged. Low 2: RoutedBatch.extractTableName and createRoutingKey split the topic on the first two dots only, so a table named a.b keeps its own routing group instead of sharing one with table a. Test: RoutedBatchTest.testDottedTableNameIsNotSplitFurther. Question (PK rebuild completeness check): not a bug. The retired side joins the key map exactly as the copy does, so a row deleted at the source before the key map was loaded is excluded from both, and the rebuilt side is SELECT DISTINCT over T without FINAL or a delete filter, so a key present only as a tombstone counts as present (PrimaryKeyBackfill.completenessCheck). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Merged
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.
Addresses the review of #1437 (comment 6088266931).
High: an offset-ack failure in a coalesced write orphaned the remaining groups.
Low 1: offset-bookkeeping failures were logged as ClickHouse errors. A failure to report written rows is now wrapped in OffsetAcknowledgementException, and run() logs it as an offset acknowledgement error before the ClickHouse error classifier runs, instead of Retriable ClickHouse error (Code: -1, Category: UNKNOWN). Retry and backoff are unchanged; the next drain retries the acknowledgement. Spec 10.01 is updated.
Low 2: dotted table names shared a routing group. RoutedBatch.extractTableName and createRoutingKey now split the topic on the first two dots only, so a table named a.b gets routing key db.a.b instead of colliding with table a. Test: RoutedBatchTest.testDottedTableNameIsNotSplitFurther. Spec 03.07 is updated.
Question (PK rebuild completeness check): not a bug.
Whole-Change-Verification: ran=JDK 17 and Maven offline. sink-connector full suite: 768 tests, 0 failures, 1 error. sink-connector-lightweight full suite: 726 tests, 0 failures, 1 error. Both errors come from a sandbox with no Docker and no network (DualJdbcDriverEndToEndIT and BinlogConnectionGuardTest.aPeerCloseOnARealSocketThrows) and occur without this change too. Targeted reruns of CoalescedQueuedBatchesTest (7), RoutedBatchTest (24) and OffsetAcknowledgementFailureTest (1) passed. Fail-without-fix check on the new mark-failure test. scripts/validate_specs.py --changed-base on 7f3102d: 83 specs, 1243 citations, PASSED; result=pass
Created on behalf of the team that co-maintains this connector; maintainers own the merge.
Generated with Claude Code (https://claude.com/claude-code)