Repository navigation
fix(resilience): refill the engine retry budget on progress, not on a bare start (Spec 10.04 section 3.5 rule 5) - #1489
Merged
Conversation
… 10.04 section 3.5 rule 4) A dead worker whose cause chain is FATAL by the error classifier (an unrepresentable value under clamp.out.of.range=false, an unknown table or column, a type mismatch, a denied privilege) recreated the engine; the engine started, connectorStarted() refilled the retry budget, the same batch was redelivered to the same outcome and the worker died again. The log read 'Restarting the engine - retry 1 of 10' every SLEEP_TIME, forever -- measured every 12-25 s for hours on two connectors -- each turn re-reading the whole schema history from the target, re-issuing the startup catalog queries and re-logging every skipped row event with its full row image. The terminal exit was never reached and no supervisor was ever signalled. handleEngineCompletion now routes a failure that ClickHouseErrorClassifier.classify marks FATAL straight to onTerminalFailure: exit code 3, or the loud liveness failure with exit.on.terminal.failure=false. Only Exceptions are classified; an Error keeps the retry path, as do retriable and unclassifiable failures. Spec 10.04 section 3.5 gains rule 4 and the matching contract bullet. Tests: TerminalFailureExitTest fatalTerminalTypeIsNotRetried, fatalErrorCodeIsNotRetried, retriableFailureStillRetries.
… bare start (Spec 10.04 section 3.5 rule 5) The completion callback recreates a failed engine up to errors.max.retries times "in a row". The previous fix for the never-refilling budget reset numRetries in the connectorStarted callback -- so a start counted as a recovery. An engine that dies on a deterministic error (an unrepresentable value in the first batch after the committed offset under the loud-clamp default, a target table that no longer exists) starts cleanly every time, streams to the same record and stops again. Every failure therefore logged "Restarting the engine - retry 1 of 10": the budget was never spent, the terminal path never ran, and the connector recreated its engine every SLEEP_TIME for as long as it was left alone, with replication stopped, the offset frozen and /status reporting Replica_Running=true. Observed on two deployments after upgrading onto the loud-clamp default: 28 restarts in six minutes, all "retry 1 of 10". Rule 4 (the preceding commit) stops the FATAL-classified case at once. This commit fixes the refill signal itself, so every other failure shape -- an exception type the classifier does not list, a cause chain carrying no ClickHouse error code, an Error -- that recurs on every start is still bounded by MAX_RETRIES instead of looping forever. The budget now refills on PROGRESS. DebeziumOffsetManagement counts every offset acknowledged to Debezium (markBatchFinished returned) -- written units and control records alike -- in a monotone counter that reset() does not touch. handleEngineCompletion reads it on every failure: a reading that differs from the one taken at the previous failure means the engine committed an offset in between -- it had recovered -- and numRetries restarts from 0 (an INFO line says so). An unchanged reading is one more failure in the same row, however cleanly the engine came up. markEngineStarted() only marks replication running for /status. Heartbeats are on by default, so an idle source still proves progress through its control-record commits. Spec 10.04 section 3.5 gains point 4 and the progress contract; the codebase mapping names the counter; doc/configuration.md describes the refill rule under exit.on.terminal.failure. Tests (TerminalFailureExitTest): - progressResetsTheBudget: an offset acknowledged through the real DebeziumOffsetManagement.acknowledgeRecords path between two failures restores the full budget (replaces successfulStartResetsTheBudget). - startWithoutProgressDoesNotResetTheBudget: every retry brings the engine up and it fails again with nothing acknowledged; the exit hook fires after exactly MAX_RETRIES restarts. Fails on the previous code (the loop never ends). - progressBeforeAnyFailureDoesNotWidenTheBudget: progress before the first failure is not a recovery; the budget is exactly MAX_RETRIES.
Collaborator
Author
|
Mutation check: with DebeziumChangeEventCapture.java reverted to the #1487 version (connectorStarted still refilling the budget) and the new tests kept, TerminalFailureExitTest runs 9 with 1 failure -- startWithoutProgressDoesNotResetTheBudget: 'no further restart once the budget is spent ==> expected: 3 but was: 4' (MAX_RETRIES=3 in the test; the engine is restarted again after the budget should have been spent, i.e. the loop). With this PR's code the same 9 tests pass. So the test pins the fix. |
Resolves the conflicts with the squash-merged #1487 (a FATAL failure is terminal at once) and #1488 (per-batch progress lines at DEBUG): the acknowledgement counter increments next to the now-DEBUG 'BATCH marked as processed' line, and spec 10.04 section 3.5 keeps the progress-refill contract (rule 5) in place of the superseded start-resets-the-budget line.
Collaborator
Author
|
Conflicts with 2.11.0 resolved by merging the branch head (45a0f6d, after #1487 and #1488 were squash-merged) into this branch -- no history rewrite. Resolution: DebeziumChangeEventCapture and TerminalFailureExitTest keep this branch's progress-refill contract (rule 5) over the superseded start-resets-the-budget line; DebeziumOffsetManagement.acknowledgeRecords keeps both the acknowledgement counter increment (this PR) and the DEBUG-level 'BATCH marked as processed' line (#1488); spec 10.04 section 3.5 keeps the rule 5 contract text. Verified on the merge commit: sink-connector DebeziumOffsetManagementTest, PreparedStatementExecutorBatchLogLevelTest, OffsetHandoffOrderTest, ParkedBatchWrittenOnceTest -- Tests run: 12, Failures: 0; sink-connector-lightweight TerminalFailureExitTest -- Tests run: 9, F 8000 ailures: 0; scripts/validate_specs.py --changed-base origin/2.11.0 -- exit 0. |
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.
Stacked on #1487 (its commit is the first of the two here; this PR shrinks to one commit once #1487 merges).
Problem
The completion callback recreates a failed engine up to errors.max.retries times 'in a row'. The earlier fix for the never-refilling budget reset numRetries in the connectorStarted callback, so a START counted as a recovery. An engine that dies on a deterministic error (an unrepresentable value in the first batch after the committed offset under the loud-clamp default, a target table that no longer exists) starts cleanly every time, streams to the same record and stops again. Every failure therefore logged 'Restarting the engine - retry 1 of 10': the budget was never spent, the terminal path never ran, and the connector recreated its engine every SLEEP_TIME for as long as it was left alone, with replication stopped, the offset frozen and /status reporting Replica_Running=true. Observed on two deployments after upgrading onto the loud-clamp default: 28 restarts in six minutes, all 'retry 1 of 10'.
#1487 (rule 4) stops the FATAL-classified case at once. This PR fixes the refill signal itself, so every other failure shape -- an exception type the classifier does not list, a cause chain carrying no ClickHouse error code, an Error -- that recurs on every start is still bounded by MAX_RETRIES instead of looping forever.
Change
The budget now refills on PROGRESS. DebeziumOffsetManagement counts every offset acknowledged to Debezium (markBatchFinished returned) -- written units and control records alike -- in a monotone counter that reset() does not touch (acknowledgements()). handleEngineCompletion reads it on every failure: a reading that differs from the one taken at the previous failure means the engine committed an offset in between -- it had recovered -- and numRetries restarts from 0 (an INFO line says so). An unchanged reading is one more failure in the same row, however cleanly the engine came up. markEngineStarted() only marks replication running for /status. Heartbeats are on by default (DEFAULT_HEARTBEAT_INTERVAL_MS), so an idle source still proves progress through its control-record commits.
Spec 10.04 section 3.5 gains point 5 and the progress contract; the codebase mapping names the counter; doc/configuration.md describes the refill rule under exit.on.terminal.failure.
Tests (TerminalFailureExitTest)