fix(spanController): ensure span consistency for operators after maintainer restart #3691

wlwilliamx · 2025-12-18T06:02:34Z

What problem does this PR solve?

Issue Number: close #3411

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Questions

Will it cause performance regression or break compatibility?

None

Do you need to update user documentation, design documentation or monitoring documentation?

None

Release note

Please refer to [Release Notes Language Style Guide](https://pingcap.github.io/tidb-dev-guide/contribute-to-tidb/release-notes-style-guide.html) to write a quality release note.

If you don't think this PR needs a release note then fill it with `None`.

…e, split after maintainer restart

ti-chi-bot · 2025-12-18T06:02:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sdojjy for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gemini-code-assist · 2025-12-18T06:03:20Z

Summary of Changes

Hello @wlwilliamx, I'm Gemini Code Assist¹! 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 consistency issue within the scheduler where the state of active dispatcher operators could be lost or become inconsistent following a maintainer restart. By enhancing the Protocol Buffer definitions to include operator-specific metadata and implementing robust tracking mechanisms within the DispatcherManager, the system can now accurately persist and restore the state of these operators. This ensures that operations on table spans, such as adding, removing, or moving dispatchers, are correctly resumed and maintained, leading to a more resilient and consistent scheduling process.

Highlights

Operator State Persistence: Introduced mechanisms to persist and restore the state of in-progress dispatcher operators (add, remove, move, split, merge) across maintainer restarts, ensuring consistency.
Protocol Buffer Updates: Added a new OperatorType enum and fields to DispatcherConfig (enabledSplit) and ScheduleDispatcherRequest (operatorType) in the heartbeatpb protobuf definitions to support operator tracking.
Dispatcher Manager Enhancements: The DispatcherManager now uses sync.Map to track currentOperatorMap and redoCurrentOperatorMap, preventing duplicate operator scheduling and ensuring proper cleanup.
Bootstrap Operator Restoration: During maintainer bootstrap, existing operators are now restored from the MaintainerBootstrapResponse, allowing the system to resume operations on previously active table spans.
Operator Type Integration: All relevant operator creation and message sending functions across maintainer/operator and maintainer/replica packages have been updated to utilize the new OperatorType.

🧠 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.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces changes to ensure span consistency for operators after a maintainer restart. The main changes involve adding an enabledSplit flag to dispatchers and tracking ongoing operators to restore them during bootstrap. My review focuses on ensuring correctness, consistency, and robustness of these new mechanisms. I've identified a few areas for improvement, including a typo, a misleading comment, a copy-paste error in a log message, and an unhandled error. Overall, the changes are well-structured and address the intended problem.

maintainer/maintainer_controller_bootstrap.go

downstreamadapter/dispatcher/redo_dispatcher.go

downstreamadapter/dispatchermanager/dispatcher_manager.go

downstreamadapter/dispatcherorchestrator/dispatcher_orchestrator.go

wlwilliamx · 2025-12-18T07:15:42Z

/test pull-integration-test

ti-chi-bot · 2025-12-18T07:15:45Z

@wlwilliamx: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-build

/test pull-cdc-kafka-integration-heavy

/test pull-cdc-kafka-integration-light

/test pull-cdc-mysql-integration-heavy

/test pull-cdc-mysql-integration-light

/test pull-cdc-storage-integration-heavy

/test pull-cdc-storage-integration-light

/test pull-check

/test pull-error-log-review

/test pull-unit-test

The following commands are available to trigger optional jobs:

/test pull-build-next-gen

/test pull-cdc-kafka-integration-heavy-next-gen

/test pull-cdc-kafka-integration-light-next-gen

/test pull-cdc-mysql-integration-heavy-next-gen

/test pull-cdc-mysql-integration-light-next-gen

/test pull-cdc-pulsar-integration-light

/test pull-cdc-pulsar-integration-light-next-gen

/test pull-cdc-storage-integration-heavy-next-gen

/test pull-cdc-storage-integration-light-next-gen

/test pull-unit-test-next-gen

Use /test all to run the following jobs that were automatically triggered:

pull-build

pull-build-next-gen

pull-check

pull-error-log-review

pull-unit-test

pull-unit-test-next-gen

Details

In response to this:

/test pull-integration-test

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 kubernetes-sigs/prow repository.

wlwilliamx · 2025-12-18T07:16:20Z

/test pull-cdc-mysql-integration-heavy

wlwilliamx · 2025-12-18T07:16:28Z

/test pull-cdc-mysql-integration-light

…st-due-to-maintainer-move-operator-lost

wlwilliamx · 2025-12-18T08:45:49Z

/test pull-cdc-mysql-integration-heavy

wlwilliamx · 2025-12-18T10:59:54Z

/test pull-cdc-mysql-integration-heavy

hongyunyan · 2025-12-19T03:35:53Z

downstreamadapter/dispatcher/basic_dispatcher.go

 	// or just a part of the table (span). When true, the dispatcher handles the entire table;
 	// when false, it only handles a portion of the table.
 	isCompleteTable bool
+	enabledSplit    bool


why we need this field?

…st-due-to-maintainer-move-operator-lost

wlwilliamx · 2025-12-22T09:37:14Z

/test pull-cdc-mysql-integration-heavy

downstreamadapter/dispatchermanager/dispatcher_manager.go

hongyunyan · 2025-12-24T03:53:41Z

downstreamadapter/dispatcherorchestrator/dispatcher_orchestrator.go

+				)
+			}
+
+			response.Operators = append(response.Operators, &heartbeatpb.ScheduleDispatcherRequest{


Please not use ScheduleDispatcherRequest here. ScheduleDispatcherRequest is for the add/remove scheduler action from maintainer to dispatchers. Please use a new message type here

Perhaps I could rename ScheduleDispatcherRequest to ScheduleDispatcherOperator, what do you think? Since the fields needed in the response are the same as those in the request, there's no need to create a separate one.

I still think it's better to use a sepereate message type. For each usage, we use a seperater message could make code more readable, and message's meaning more clear

If we use a separate struct, the code would require many unnecessary struct conversions. I think simply renaming the existing ScheduleDispatcherRequest to indicate that it's a struct used to store operator information would be sufficient.

downstreamadapter/dispatcherorchestrator/dispatcher_orchestrator.go

hongyunyan · 2025-12-24T05:00:22Z

downstreamadapter/dispatchermanager/helper.go

+			continue
+		}
+		// If there is already an operator for the span, skip this request.
+		_, exists := dispatcherManager.currentOperatorMap.Load(operatorKey)


Consider this scenario: a dispatcher is performing a merge operation, but before it's finished, a drop table DDL causes the dispatcher to be removed. In this situation, could it happen that the dispatcher holding the operator has now received a new operator?

when will these continue happens? It will make the create or remove action skipped. Is there no correctness issue here?

The merge operator will store the data in another map, which will not conflict with this one. You can refer to the PR of another merge operator.

downstreamadapter/dispatchermanager/helper.go

maintainer/maintainer_controller_bootstrap.go

hongyunyan · 2025-12-24T05:14:16Z

maintainer/maintainer_controller_bootstrap.go

+							}
+						},
+					)
+					if ok := c.operatorController.AddOperator(op); !ok {


Could we just use pushOperator instead of AddOperator

Are there any problems using AddOperator?

pushOperator has more complete functions with metrics, log and also start() function.

AddOperator will do some checks and call the pushOperator

…ainer-move-operator-lost' into fix/dispatcher-lost-due-to-maintainer-move-operator-lost

…st-due-to-maintainer-move-operator-lost

wlwilliamx · 2026-01-04T13:49:31Z

/test pull-cdc-mysql-integration-heavy

wlwilliamx · 2026-01-04T13:49:54Z

/test pull-cdc-mysql-integration-light

wlwilliamx · 2026-01-04T16:06:59Z

/retest

wlwilliamx · 2026-01-05T04:04:30Z

/test all

wlwilliamx · 2026-01-05T15:18:27Z

/retest

ti-chi-bot · 2026-01-05T16:59:47Z

@wlwilliamx: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-error-log-review	`7e8b71a`	link	true	`/test pull-error-log-review`
pull-cdc-storage-integration-heavy	`7e8b71a`	link	true	`/test pull-cdc-storage-integration-heavy`

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

fix(scheduler): ensure span consistency for operator add, remove, mov…

2686e4a

…e, split after maintainer restart

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 18, 2025

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/needs-triage-completed labels Dec 18, 2025

wlwilliamx changed the title ~~fix(scheduler): ensure span consistency for operators after maintainer restart~~ fix(spanController): ensure span consistency for operators after maintainer restart Dec 18, 2025

wlwilliamx requested a review from hongyunyan December 18, 2025 06:04

gemini-code-assist bot reviewed Dec 18, 2025

View reviewed changes

wlwilliamx added 6 commits December 18, 2025 15:18

fix a typo

7288257

fix a log typo

b9637e2

handle err for restoreCurrentWorkingOperators

ce070cc

modify a comment

b0929db

Merge remote-tracking branch 'upstream/master' into fix/dispatcher-lo…

d8732bc

…st-due-to-maintainer-move-operator-lost

resolve conflicts

050e13f

fix newRemoveDispatcherOperator node id

a7be949

hongyunyan reviewed Dec 19, 2025

View reviewed changes

wlwilliamx added 4 commits December 19, 2025 17:14

handle return value for AddOperator and add some logs for it

c9891d8

add some logs for dispatcher manager store current working operators

8c4e099

fix remove operator nil span

75638b2

Merge remote-tracking branch 'upstream/master' into fix/dispatcher-lo…

82637b9

…st-due-to-maintainer-move-operator-lost

remove enabled split

26f5a83

wlwilliamx added 4 commits December 23, 2025 20:31

some fix after review

fc4c16c

add move_table_maintainer_failover integration test

aa9cf0d

disable it from ci

4ad6818

use failpoint script

bec938c

hongyunyan reviewed Dec 24, 2025

View reviewed changes

wlwilliamx added 19 commits December 24, 2025 18:10

modify style

40ec63f

refactor createBootstrapResponse

725fe3c

refactor dispatcher manager handle opeartor request

4f6661b

add comments for dispatcher manager handle operators

9d78e86

add more comments for dispatcher manager handle operators

e71913d

refactor restoreCurrentWorkingOperators and add some comments

d7fdcb0

Merge remote-tracking branch 'origin/fix/dispatcher-lost-due-to-maint…

9660e02

…ainer-move-operator-lost' into fix/dispatcher-lost-due-to-maintainer-move-operator-lost

fix currentOperatorMap key

0c71d10

Merge remote-tracking branch 'upstream/master' into fix/dispatcher-lo…

8fcdf20

…st-due-to-maintainer-move-operator-lost

fix currentOperatorMap type

40793fd

Merge remote-tracking branch 'upstream/master' into fix/dispatcher-lo…

cdeb1a4

…st-due-to-maintainer-move-operator-lost

fix move operator original type

ab566e8

reuse working span

e762866

Merge remote-tracking branch 'upstream/master' into fix/dispatcher-lo…

834f5ba

…st-due-to-maintainer-move-operator-lost

make check

d5288d7

refactor test

2d32ee9

add wait control for move table api

cc05bc6

add a comment for test

958a989

remove the wrong final wait_for_table_on_addr

7e8b71a

fix(spanController): ensure span consistency for operators after maintainer restart #3691

Are you sure you want to change the base?

fix(spanController): ensure span consistency for operators after maintainer restart #3691

Conversation

wlwilliamx commented Dec 18, 2025

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

Uh oh!

ti-chi-bot bot commented Dec 18, 2025

Uh oh!

gemini-code-assist bot commented Dec 18, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wlwilliamx commented Dec 18, 2025

Uh oh!

ti-chi-bot bot commented Dec 18, 2025

Uh oh!

wlwilliamx commented Dec 18, 2025

Uh oh!

wlwilliamx commented Dec 18, 2025

Uh oh!

wlwilliamx commented Dec 18, 2025

Uh oh!

wlwilliamx commented Dec 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wlwilliamx commented Dec 22, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wlwilliamx commented Jan 4, 2026

Uh oh!

wlwilliamx commented Jan 4, 2026

Uh oh!

wlwilliamx commented Jan 4, 2026

Uh oh!

wlwilliamx commented Jan 5, 2026

Uh oh!