maintainer,dispatcher: match barrier done with action#4389
maintainer,dispatcher: match barrier done with action#43893AceShowHand wants to merge 10 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
the
size/XXL
|
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:
📝 WalkthroughWalkthroughIntroduces an asynchronous "prepared" pathway for block events in the dispatcher (writePrepared/passPrepared), moves block-event IO onto a dedicated executor, removes the Flush action from the heartbeat proto, and removes per-barrier flush flags and related flush-phase logic across barrier/maintainer code and tests. Tests and mocks updated to support deferred flush hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant Dispatcher
participant Executor
participant Sink
participant Maintainer
participant Barrier
Dispatcher->>Executor: enqueue writePreparedBlockEvent(event)
Executor->>Sink: FlushDMLBeforeBlock(event) (may block via test hook)
Executor->>Sink: WritePreparedBlockEvent to sink (DDL/SyncPoint path)
Executor->>Maintainer: reportBlockedEventDone(actionCommitTs, actionIsSyncPoint, ...)
Maintainer->>Barrier: update BlockEvent status (phase evaluation)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in the barrier advancement mechanism, particularly in split-table move/merge scenarios. Previously, the system could incorrectly advance a barrier if a duplicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/test all |
![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 effectively addresses a critical bug where a duplicated Flush DONE message could be misinterpreted as a Write completion, leading to premature barrier advancement. The introduction of the DoneAction enum to explicitly tag the source of a DONE status is a robust solution. The changes are well-implemented across the dispatcher and maintainer, and the inclusion of backward compatibility for mixed-version clusters is thoughtful. The new unit tests, especially TestBarrierIgnoresFlushDoneWhileWaitingWrite, are excellent and directly verify the fix for the described race condition. I have one suggestion to improve the clarity of the legacy action inference logic.
Note: Security Review did not run due to the size of the PR.
|
/test all |
3 similar comments
|
/test all |
|
/test all |
|
/test all |
|
/retest |
|
/test all |
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
|
/check-issue-triage-complete |
|
/test all |
1 similar comment
|
/test all |
|
/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 the current code and only fix it if needed.
Inline comments:
In `@maintainer/barrier_phase_test.go`:
- Around line 1-13: The file barrier_phase_test.go is missing the standard
Apache 2.0 copyright header causing CI failure; add the same multi-line
copyright header used across the repo at the very top of barrier_phase_test.go
(before the package statement) so that package maintainer and its test files
match the project's header format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f971d28-bf78-4add-b07e-5b0db0af6ad0
📒 Files selected for processing (6)
maintainer/barrier.gomaintainer/barrier_event.gomaintainer/barrier_event_test.gomaintainer/barrier_phase_test.gomaintainer/barrier_test.gomaintainer/maintainer_controller_bootstrap.go
|
/test all |
ti-chi-bot
bot
added
size/XL
|
/test all |
| // It relies on PassBlockEventToSink to preserve ordering and mark the event passed. | ||
| func (d *BasicDispatcher) PassBlockEvent(pendingEvent commonEvent.BlockEvent, actionCommitTs uint64, actionIsSyncPoint bool) { | ||
| failpoint.Inject("BlockOrWaitBeforePass", nil) | ||
| err := d.PassBlockEventToSink(pendingEvent) |
There was a problem hiding this comment.
What is the difference between PassBlockEventToSink and the passPreparedBlockEvent function?
| d.PassBlockEvent(pendingEvent, actionCommitTs, actionIsSyncPoint) | ||
| }) | ||
| return true | ||
| case heartbeatpb.Action_Flush: |
| // It is required for storage sink when split-table is enabled: one table may have multiple | ||
| // dispatchers on different nodes, and every dispatcher must flush pre-barrier | ||
| // DML first to prevent it from being reordered after the writer executes DDL. | ||
| Flush = 2; |
There was a problem hiding this comment.
Will this cause compatibility issues during the upgrade process?
|
/test all |
1 similar comment
|
/test all |
|
/test all |
ti-chi-bot
bot
added
size/XXL
|
/test all |
|
/test all |
|
/test all |
2 similar comments
|
/test all |
|
/test all |
|
/retest |
|
/test all |
|
/test all |
|
@3AceShowHand: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: close #4398
#4263introducedFlushas a maintainer-visible barrier action for storage sink. That solved the ordering problem, but it also pushed storage-sink-specific flush semantics into the maintainer state machine and made the barrier protocol three-phase.That design turned out to be unnecessarily complex:
Flush -> Write -> PassFlushactionThis PR simplifies the design by moving the flush step back into dispatcher-local handling while keeping the storage-sink ordering guarantee.
What is changed and how it works?
The protocol is simplified back to
Write -> Pass.Dispatcher side:
FlushDMLBeforeBlocklocally before reportingWAITINGto maintainerflush -> WAITINGpathAddBlockEventToSink/PassBlockEventToSinkare reused as the only block-event sink helpers; whether they need to flush is inferred from dispatcher-local block status instead of a caller-passed flagWriteorPass, dispatcher does not flush again for that same pending block event, so we avoid duplicate flush / duplicate DDL executionMaintainer side:
Flushaction from the heartbeat protocol#4263maintainer logic: wait for all influenced dispatchers, sendWriteto one writer, then sendPassIn other words, flush still exists semantically, but it becomes a dispatcher-local prerequisite for entering
WAITINGinstead of a maintainer-visible phase.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No expected performance regression.
This changes the internal maintainer/dispatcher barrier protocol by removing the explicit
Flushaction. It is intended as a same-version change, not a mixed-version rolling-upgrade-compatible protocol extension.Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note