Skip to content

sole add switch for scanwindow#5371

Open
haiboumich wants to merge 1 commit into
pingcap:masterfrom
haiboumich:haiboumich/sole-add-switch-for-scanwindow
Open

sole add switch for scanwindow#5371
haiboumich wants to merge 1 commit into
pingcap:masterfrom
haiboumich:haiboumich/sole-add-switch-for-scanwindow

Conversation

@haiboumich

@haiboumich haiboumich commented Jun 12, 2026

Copy link
Copy Markdown

What problem does this PR solve?

Issue Number: close #xxx

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?
Do you need to update user documentation, design documentation or monitoring documentation?

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

Summary by CodeRabbit

  • New Features

    • Configurable adaptive "scan window" for changefeeds (disabled by default). The setting is exposed to dispatchers and included in event requests.
  • Performance

    • Memory-release tuning updated: per-area override and adjusted defaults to reduce deadlocks and allow more aggressive release when scan window is active.
  • Tests

    • Added tests verifying scan-window enabled/disabled behavior and related signaling.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-linked-issue first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 12, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown

[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 nongfushanquan 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:

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

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 944e1211-ffc0-4e8b-857b-1f4032514c5d

📥 Commits

Reviewing files that changed from the base of the PR and between dbf6f1c and 4c438e2.

⛔ Files ignored due to path filters (1)
  • eventpb/event.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (22)
  • downstreamadapter/dispatcher/basic_dispatcher.go
  • downstreamadapter/dispatcher/basic_dispatcher_info.go
  • downstreamadapter/dispatcher/event_dispatcher_test.go
  • downstreamadapter/dispatchermanager/dispatcher_manager.go
  • downstreamadapter/dispatchermanager/dispatcher_manager_test.go
  • downstreamadapter/eventcollector/dispatcher_session.go
  • downstreamadapter/eventcollector/dispatcher_stat_test.go
  • downstreamadapter/eventcollector/event_collector.go
  • downstreamadapter/eventcollector/event_collector_test.go
  • eventpb/event.proto
  • pkg/config/changefeed.go
  • pkg/config/replica_config.go
  • pkg/eventservice/dispatcher_stat.go
  • pkg/eventservice/event_broker.go
  • pkg/eventservice/event_broker_test.go
  • pkg/eventservice/event_scanner_test.go
  • pkg/eventservice/event_service.go
  • pkg/eventservice/event_service_test.go
  • pkg/eventservice/scan_window.go
  • pkg/eventservice/scan_window_test.go
  • utils/dynstream/interfaces.go
  • utils/dynstream/memory_control.go
✅ Files skipped from review due to trivial changes (1)
  • downstreamadapter/dispatchermanager/dispatcher_manager_test.go
🚧 Files skipped from review as they are similar to previous changes (21)
  • pkg/eventservice/event_service.go
  • pkg/eventservice/event_scanner_test.go
  • pkg/config/changefeed.go
  • downstreamadapter/eventcollector/dispatcher_stat_test.go
  • pkg/eventservice/dispatcher_stat.go
  • downstreamadapter/dispatcher/basic_dispatcher.go
  • pkg/eventservice/scan_window.go
  • downstreamadapter/dispatchermanager/dispatcher_manager.go
  • eventpb/event.proto
  • utils/dynstream/memory_control.go
  • downstreamadapter/eventcollector/event_collector_test.go
  • pkg/eventservice/event_service_test.go
  • pkg/eventservice/event_broker_test.go
  • downstreamadapter/eventcollector/dispatcher_session.go
  • pkg/config/replica_config.go
  • downstreamadapter/dispatcher/basic_dispatcher_info.go
  • utils/dynstream/interfaces.go
  • pkg/eventservice/scan_window_test.go
  • downstreamadapter/eventcollector/event_collector.go
  • downstreamadapter/dispatcher/event_dispatcher_test.go
  • pkg/eventservice/event_broker.go

📝 Walkthrough

Walkthrough

Adds an EnableScanWindow flag and wires it from config/proto through dispatcher initialization to event service logic and dynstream tuning, gating scan-window behaviors and updating tests.

Changes

Enable Adaptive Scan Window Feature Flag

Layer / File(s) Summary
Configuration and protobuf contracts
eventpb/event.proto, pkg/config/replica_config.go, pkg/config/changefeed.go
EnableScanWindow added to replica config (default false), changefeed config, and DispatcherRequest proto; changefeed config conversion populates the new field.
Dispatcher interface and initialization
downstreamadapter/dispatcher/basic_dispatcher.go, downstreamadapter/dispatcher/basic_dispatcher_info.go
DispatcherService gains GetEnableScanWindow() bool; SharedInfo and NewSharedInfo accept/store enableScanWindow; BasicDispatcher.GetEnableScanWindow() implemented.
Wire configuration to dispatcher and service
downstreamadapter/dispatchermanager/dispatcher_manager.go, pkg/eventservice/event_service.go
Manager passes config EnableScanWindow into NewSharedInfo; DispatcherInfo interface adds GetEnableScanWindow().
Request payload construction
downstreamadapter/eventcollector/dispatcher_session.go
Dispatcher REGISTER and RESET requests now set EnableScanWindow from dispatcher instance.
Event service conditional behaviors
pkg/eventservice/dispatcher_stat.go, pkg/eventservice/event_broker.go, pkg/eventservice/scan_window.go
changefeedStatus stores enableScanWindow; pending-DDL local advance, empty-range resolved-ts signaling, and scan-window metrics initialization are gated by the flag; updateMemoryUsage, refreshMinSentResolvedTs, and getScanMaxTs early-return when disabled.
Dynstream memory release tuning
downstreamadapter/eventcollector/event_collector.go, utils/dynstream/interfaces.go, utils/dynstream/memory_control.go
Add per-area releaseMemoryRatio setting and setter; introduce scanWindowReleaseMemoryRatio; default release ratio adjusted and deadlock watermark separated; per-area overrides used during release.
Test infrastructure updates
downstreamadapter/dispatcher/event_dispatcher_test.go, downstreamadapter/dispatchermanager/dispatcher_manager_test.go, downstreamadapter/eventcollector/dispatcher_stat_test.go, downstreamadapter/eventcollector/event_collector_test.go, pkg/eventservice/event_service_test.go, pkg/eventservice/event_scanner_test.go
Test helpers and mocks updated to accept/pass enableScanWindow and to implement GetEnableScanWindow().
New and updated tests
pkg/eventservice/event_broker_test.go, pkg/eventservice/scan_window_test.go
Added TestGetScanTaskDataRangeEmptySignalGatedByScanWindow and TestScanWindowDisabledIsNoOp; many scan-window tests updated to construct enabled statuses.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • pingcap/ticdc#4836: Both change changefeed-status construction paths and related fields used by event broker.

Suggested labels

lgtm, approved

Suggested reviewers

  • flowbehappy
  • asddongmen
  • wk989898

Poem

🐰 A flag hops through the config layers bright,
Gating scan windows left and right,
Memory whispers when the window's on,
Metrics hum and tests pass at dawn,
Rabbit cheers — both paths kept light.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is entirely the unfilled template; all sections are placeholders with no actual implementation details, issue links, test descriptions, or release notes provided. Fill in all required sections: link a specific issue, describe the changes and their purpose, specify which tests were added, answer the compatibility/documentation questions, and provide a proper release note.
Docstring Coverage ⚠️ Warning Docstring coverage is 13.16% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'sole add switch for scanwindow' is vague and uses informal terminology ('sole', 'switch') without clearly describing what the changeset does. Revise the title to be more descriptive and professional, e.g., 'Add configuration option to enable/disable adaptive scan window behavior' or similar.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ti-chi-bot ti-chi-bot Bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 12, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the enableScanWindow configuration option to conditionally gate the adaptive scan window feature (memory control and adaptive scan interval) for changefeeds, defaulting to false to preserve baseline behavior. This setting is propagated across the downstream adapter, event collector, protobuf definitions, replica configurations, and the event service. Feedback on the changes suggests correcting the struct tags for EnableScanWindow in ChangefeedConfig to ensure consistency with other configuration structs by adding the toml tag and using kebab-case for the json tag.

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.

Comment thread pkg/config/changefeed.go
EnableTableAcrossNodes bool `toml:"enable-table-across-nodes" json:"enable-table-across-nodes,omitempty"`
// EnableScanWindow controls whether the event service applies the adaptive scan
// window (memory control + adaptive scan interval) for this changefeed.
EnableScanWindow bool `json:"enable_scan_window" default:"false"`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The struct tag for EnableScanWindow in ChangefeedConfig is inconsistent with replicaConfig and other fields in ChangefeedConfig (such as EnableTableAcrossNodes). It is missing the toml tag entirely, and uses snake_case (enable_scan_window) instead of kebab-case (enable-scan-window) for the json tag.

Using kebab-case and adding the toml tag ensures consistency across configuration structs and prevents potential parsing/serialization issues.

Suggested change
EnableScanWindow bool `json:"enable_scan_window" default:"false"`
EnableScanWindow bool `toml:"enable-scan-window" json:"enable-scan-window,omitempty" default:"false"`

Add a per-changefeed `enable-scan-window` replica config (default false) and
plumb it through the changefeed config, dispatcher info and event service.

When the switch is off the adaptive scan-window feature is fully inert and the
changefeed behaves as if it was never introduced:
- event service: memory control, adaptive scan interval, base-ts capping,
  empty-range signal, pending-DDL local advance and scan-window metrics are
  all gated.
- dynstream: the memory release ratio follows the switch (0.4 off / 0.6 on);
  the deadlock high-water-mark stays 0.6 in both modes.
@haiboumich haiboumich force-pushed the haiboumich/sole-add-switch-for-scanwindow branch from dbf6f1c to 4c438e2 Compare June 13, 2026 17:06
@ti-chi-bot

ti-chi-bot Bot commented Jun 13, 2026

Copy link
Copy Markdown

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-linked-issue label, please provide the linked issue number on one line in the PR body, for example: Issue Number: close #123 or Issue Number: ref #456.

📖 For more info, you can check the "Contribute Code" section in the development guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/needs-linked-issue first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant