coordinator: handle fast-fail errors before checkpoint progress - #5424
Conversation
ti-chi-bot
Bot
added
the
release-note
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBackoff gains a new ChangesFast-fail error handling and early exit integration
Sequence Diagram(s)sequenceDiagram
participant UpdateStatus as Changefeed.UpdateStatus
participant CheckFailed as checkFailedStatus
participant FindFastFail as findFastFailError
participant Log as Logger
UpdateStatus->>UpdateStatus: Store c.status
UpdateStatus->>CheckFailed: Call checkFailedStatus(newStatus)
CheckFailed->>CheckFailed: Check if m.failed already set
CheckFailed->>FindFastFail: Scan status.Err for fast-fail
FindFastFail-->>CheckFailed: Fast-fail error found
CheckFailed->>CheckFailed: Conditionally advance checkpointTs
CheckFailed->>Log: Log fast-fail event
CheckFailed->>CheckFailed: Set m.failed = true
CheckFailed-->>UpdateStatus: Return (changed, StateFailed, err)
UpdateStatus->>UpdateStatus: Check early-return condition
UpdateStatus-->>UpdateStatus: Return immediately on StateFailed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
added
the
size/M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces fast-fail error handling in the changefeed backoff coordinator. It updates CheckStatus to detect fast-fail errors via a new helper function findFastFailError, refactors HandleError to use this helper, and adds corresponding unit tests. The review feedback suggests adding defensive checks: a nil check for status in CheckStatus to prevent potential nil pointer dereferences, and an empty-slice check in HandleError to avoid out-of-bounds panics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/test all |
3AceShowHand
changed the title
ti-chi-bot
Bot
added
needs-1-more-lgtm
ti-chi-bot
Bot
added
size/L
|
/test all |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coordinator/changefeed/changefeed.go`:
- Around line 177-180: The early return condition on line 178 in the CheckStatus
block is too broad by returning when changed is true, which skips the TargetTs
completion logic on lines 189-193. This causes incorrect state reporting when
both checkpoint recovery and target completion occur in the same heartbeat.
Narrow the if condition to only return early on actual failure outcomes by
removing the changed clause, keeping only the error check and StateFailed state
check: err != nil || state == config.StateFailed. This allows valid recovery
transitions with changed true to continue executing the TargetTs completion
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8ce2c00-95dd-4289-9d07-274dc345f644
📒 Files selected for processing (3)
coordinator/changefeed/backoff.gocoordinator/changefeed/changefeed.gocoordinator/changefeed/changefeed_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- coordinator/changefeed/backoff.go
|
/test all |
|
/test all |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongyunyan, wk989898 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/cherry-pick release-8.5 |
|
@3AceShowHand: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
4 participants
What problem does this PR solve?
Issue Number: close #5246
Maintainer heartbeat can carry both checkpoint progress and an unretryable error in the same
MaintainerStatus. Before this PR,Backoff.CheckStatushandled checkpoint progress before checkingstatus.Err, so a fast-fail error such asErrTableRouteConflictcould be ignored when the checkpoint advanced in the same report. Since maintainer-reported errors are transient, the changefeed could remain normal instead of moving to failed.This was observed in the table route conflict detection flow: the route conflict was detected and reported by the maintainer, but coordinator treated the heartbeat as normal progress and only persisted the checkpoint.
What is changed and how it works?
Backoff.CheckStatus.CheckStatusandHandleError.ErrTableRouteConflictand for fast-fail errors reported together with checkpoint progress.This PR intentionally does not change retryable-error precedence. Retryable errors that arrive with checkpoint progress still follow the existing recovery semantics; that broader behavior should be handled separately if needed.
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No. The change only reorders coordinator-side classification for fast-fail / unretryable errors in maintainer status handling.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note
Summary by CodeRabbit
Release Notes