server, dispatcher: improve node liveness self fence#5106
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds coordinated local fencing: DispatcherManager write-path fencing and lifecycle refactor, DispatcherOrchestrator fenced mode and propagation, a server session watchdog that triggers local fence on etcd lease/session loss, unit/integration tests, helper updates, and related CI/script/failpoint changes. ChangesDispatcherManager: core fencing and lifecycle
DispatcherOrchestrator: fenced mode and manager tracking
Server: session watchdog
Integration tests and CI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 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)
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/XL
![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 a local fencing mechanism to ensure that downstream writes are stopped immediately when a capture loses its etcd session or lease. This is achieved by adding a session watchdog in the server and implementing a LocalFence method across the DispatcherOrchestrator and DispatcherManager to bypass graceful draining in failure scenarios. The review feedback identifies potential nil pointer dereferences in the DispatcherManager's shutdown logic, specifically regarding the redoSink when redo logging is enabled.
|
/test all |
ti-chi-bot
Bot
added
size/XXL
Signed-off-by: dongmen <414110582@qq.com>
Signed-off-by: dongmen <414110582@qq.com>
asddongmen
force-pushed
the
0520-improve-node-liveness-self-fence
branch
from
4a22383 to
fa0781c
Compare
|
/test all |
|
/test all |
|
/test all |
|
/test pull-cdc-mysql-integration-heavy |
|
/test all |
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
/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
🧹 Nitpick comments (1)
downstreamadapter/dispatcherorchestrator/dispatcher_orchestrator_test.go (1)
458-462: ⚡ Quick winMake the drop assertion deterministic.
Lines 458-462 only show that nothing reached
processedwithin 50ms. A message that was merely delayed, or enqueued after fence but never drained, looks identical, so this can both mask regressions and flake on slow CI. Please assert the post-fence admission state directly instead of relying on a short timeout.As per coding guidelines,
**/*_test.go: Prefer focused deterministic tests; see docs/agents/testing.md before adding or changing tests.🤖 Prompt for 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. In `@downstreamadapter/dispatcherorchestrator/dispatcher_orchestrator_test.go` around lines 458 - 462, Replace the flaky 50ms timeout check on the processed channel with a deterministic assertion of post-fence admission state: after applying the local fence, directly assert the orchestrator/maintainer admission flag or queue length (e.g. check an "isAdmitting" / "admissionOpen" boolean or the maintainer queue length) to prove new maintainer messages are rejected, and if only the processed channel is available, use a non-blocking select (case msg := <-processed: require.FailNow(...); default: ) and also attempt to enqueue a new maintainer and assert that enqueue returns a failure/false or does not increase the queue size; reference the processed channel and the local fence operation in the test to locate where to make this change.
🤖 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 `@downstreamadapter/dispatchermanager/dispatcher_manager.go`:
- Around line 60-77: Create a dedicated predefined repo error for the fenced
write-path state (e.g., add errors.ErrDispatcherWritePathClosed in the central
errors package) and replace the current message-substring approach: have
newWritePathClosedError() generate/return that specific error (use the repo
error construction consistent with other errors, e.g., FastGenByArgs on
errors.ErrDispatcherWritePathClosed), and update IsWritePathClosedError(err) to
detect the condition by matching that error directly (either via errors.Is(err,
errors.ErrDispatcherWritePathClosed) or comparing RFCCode to
errors.ErrDispatcherWritePathClosed.RFCCode()) instead of checking for
ErrChangefeedInitTableTriggerDispatcherFailed plus a substring.
---
Nitpick comments:
In `@downstreamadapter/dispatcherorchestrator/dispatcher_orchestrator_test.go`:
- Around line 458-462: Replace the flaky 50ms timeout check on the processed
channel with a deterministic assertion of post-fence admission state: after
applying the local fence, directly assert the orchestrator/maintainer admission
flag or queue length (e.g. check an "isAdmitting" / "admissionOpen" boolean or
the maintainer queue length) to prove new maintainer messages are rejected, and
if only the processed channel is available, use a non-blocking select (case msg
:= <-processed: require.FailNow(...); default: ) and also attempt to enqueue a
new maintainer and assert that enqueue returns a failure/false or does not
increase the queue size; reference the processed channel and the local fence
operation in the test to locate where to make this change.
🪄 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: 54d91177-8133-4394-a936-6f0327fa9be2
📒 Files selected for processing (8)
downstreamadapter/dispatchermanager/dispatcher_manager.godownstreamadapter/dispatchermanager/dispatcher_manager_redo.godownstreamadapter/dispatchermanager/dispatcher_manager_test.godownstreamadapter/dispatchermanager/helper.godownstreamadapter/dispatcherorchestrator/dispatcher_orchestrator.godownstreamadapter/dispatcherorchestrator/dispatcher_orchestrator_test.gotests/integration_tests/capture_local_fence_on_session_done/run.shtests/integration_tests/capture_session_done_during_task/run.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration_tests/capture_local_fence_on_session_done/run.sh
- downstreamadapter/dispatcherorchestrator/dispatcher_orchestrator.go
|
/test all |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
downstreamadapter/dispatchermanager/dispatcher_manager_redo.go (1)
338-340: Document fenced no-op inUpdateRedoMeta.
DispatcherManager.UpdateRedoMeta(downstreamadapter/dispatchermanager/dispatcher_manager_redo.go:338-340) returns immediately whenwritePathClosedis set with no error/log. The only observed call site (downstreamadapter/dispatchermanager/helper.go:751-752) ignores the outcome, so callers can’t tell whether the meta was updated. Add a brief comment above the early return explaining this is an intentional no-op during write-path shutdown/fencing.🤖 Prompt for 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. In `@downstreamadapter/dispatchermanager/dispatcher_manager_redo.go` around lines 338 - 340, UpdateRedoMeta currently returns immediately when e.writePathClosed.Load() is true with no indication to readers; add a brief comment directly above the early return in function UpdateRedoMeta explaining that this is an intentional no-op because the write path is closed/fenced during shutdown so metadata updates are ignored (do not change behavior or add an error/log, just document the intent).
🤖 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.
Nitpick comments:
In `@downstreamadapter/dispatchermanager/dispatcher_manager_redo.go`:
- Around line 338-340: UpdateRedoMeta currently returns immediately when
e.writePathClosed.Load() is true with no indication to readers; add a brief
comment directly above the early return in function UpdateRedoMeta explaining
that this is an intentional no-op because the write path is closed/fenced during
shutdown so metadata updates are ignored (do not change behavior or add an
error/log, just document the intent).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fbd800b-16e6-49af-9547-7a196144a232
📒 Files selected for processing (5)
downstreamadapter/dispatcher/event_dispatcher.godownstreamadapter/dispatchermanager/dispatcher_manager.godownstreamadapter/dispatchermanager/dispatcher_manager_redo.godownstreamadapter/dispatchermanager/dispatcher_manager_test.godownstreamadapter/dispatchermanager/helper.go
🚧 Files skipped from review as they are similar to previous changes (2)
- downstreamadapter/dispatchermanager/helper.go
- downstreamadapter/dispatchermanager/dispatcher_manager.go
|
/test all |
|
/retest |
1 similar comment
|
/retest |
|
/test all |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
downstreamadapter/dispatchermanager/dispatcher_manager_redo.go (1)
99-108: 💤 Low valueRedundant fence check after redo message registration.
The check at lines 106-108 occurs after
RegisterRedoMessageDshas already completed at line 104. If the write path closes between lines 105 and 106, the registration has already happened. This differs from other fencing patterns where the check prevents the operation. Consider removing this redundant check or moving the registration inside the critical section.🤖 Prompt for 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. In `@downstreamadapter/dispatchermanager/dispatcher_manager_redo.go` around lines 99 - 108, The post-registration fence check is redundant; either remove the second manager.writePathClosed.Load() check and its newWritePathClosedError() return, or (preferred) move the appcontext.GetService[*HeartBeatCollector](appcontext.HeartbeatCollector).RegisterRedoMessageDs(manager) call inside the critical section protected by manager.writePathMu so the registration is only performed while the write-path is provably open; then release manager.writePathMu and keep the single pre-registration check using manager.writePathClosed.Load() to decide whether to return newWritePathClosedError().
🤖 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.
Nitpick comments:
In `@downstreamadapter/dispatchermanager/dispatcher_manager_redo.go`:
- Around line 99-108: The post-registration fence check is redundant; either
remove the second manager.writePathClosed.Load() check and its
newWritePathClosedError() return, or (preferred) move the
appcontext.GetService[*HeartBeatCollector](appcontext.HeartbeatCollector).RegisterRedoMessageDs(manager)
call inside the critical section protected by manager.writePathMu so the
registration is only performed while the write-path is provably open; then
release manager.writePathMu and keep the single pre-registration check using
manager.writePathClosed.Load() to decide whether to return
newWritePathClosedError().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0fbe318-a1e2-47e4-b42a-b2f585006fd8
📒 Files selected for processing (5)
downstreamadapter/dispatchermanager/dispatcher_manager.godownstreamadapter/dispatchermanager/dispatcher_manager_redo.godownstreamadapter/dispatchermanager/dispatcher_manager_test.godownstreamadapter/dispatcherorchestrator/dispatcher_orchestrator.godownstreamadapter/dispatcherorchestrator/dispatcher_orchestrator_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand, 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:
|
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
Signed-off-by: dongmen <414110582@qq.com>
|
/test all |
asddongmen
removed
the
do-not-merge/hold
|
/cherry-pick release-nextgen-202603 |
|
@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 #5202
When a TiCDC capture loses its etcd session or lease, it can no longer prove that it still owns local dispatcher work. The previous shutdown path did not immediately fence local write paths, so a stale capture could still accept maintainer requests or continue writing downstream while failover was already in progress. This created a short but unsafe window for duplicate or out-of-date downstream writes before normal cleanup finished.
What is changed and how it works?
This PR adds a local fence path for session-done and lease-expired events. The server watches the etcd session, triggers local fencing before shutdown, and the dispatcher orchestrator stops accepting new maintainer requests. Dispatcher managers cancel local write paths immediately, close local dispatchers asynchronously, and continue cleanup without waiting for progress draining. The redo cleanup path is also guarded so partially initialized managers do not panic when fencing. New integration coverage simulates capture session loss and verifies the local fence behavior and downstream consistency.
Check List
Tests
/test allQuestions
Will it cause performance regression or break compatibility?
No. The new path only runs when a local capture loses its session or is being fenced.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests