*: add changefeed-level performance mode - #5862
Conversation
Keep the low-latency mode focused on setting the logpuller advance interval to zero. Remove the additional batched heap update path and its helper after the 10k-region E2E test showed comparable mean and p95 latency without it.
Replace the periodic schema-capped scan retry with applied SchemaStore notifications. Serialize dispatcher scan scheduling with a short-lock state machine and coalesce worker continuations without dropping queued work.
Keep no-DML/no-DDL resolved notifications out of the scan worker queue while preserving dispatcher scan ownership. Gate continuation and schema-blocked recovery on low-latency mode, and cover queue-full recovery with a dropped-task metric.
…level-low-latency-mode
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThe change adds validated throughput and low-latency performance modes. It propagates the mode through API, dispatcher, and event requests. It adjusts heartbeat and maintainer notifications. Coordinator metrics now publish after complete reporting rounds. ChangesPerformance mode configuration
Dispatcher and maintainer behavior
Coordinator metrics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DispatcherManager
participant BasicDispatcher
participant DispatcherSession
participant DispatcherRequest
DispatcherManager->>BasicDispatcher: initialize low-latency mode
BasicDispatcher->>DispatcherSession: expose low-latency mode
DispatcherSession->>DispatcherRequest: set low-latency mode on register or reset
sequenceDiagram
participant Maintainer
participant MaintainerManager
participant Coordinator
Maintainer->>Maintainer: accept newer watermark
Maintainer->>MaintainerManager: send coalesced heartbeat notification
MaintainerManager->>Maintainer: process maintainer heartbeat
Coordinator->>Coordinator: publish metrics after complete reporting round
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
/test all |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
pkg/eventservice/event_broker.go (2)
1182-1199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the keyspace key construction.
getSchemaBlockedDispatcherBucketandremoveSchemaBlockedDispatcherbuild the samecommon.KeyspaceMetafrom the same two sources.getScanTaskRequestResult(Lines 457-460) andgetSchemaBlockedDispatcherBucketalso repeat it. A single helper ondispatcherStatwould keep the key definition in one place.♻️ Sketch: one keyspace-key helper
+func (a *dispatcherStat) keyspaceMeta() common.KeyspaceMeta { + return common.KeyspaceMeta{ + ID: a.info.GetTableSpan().KeyspaceID, + Name: a.changefeedStat.changefeedID.Keyspace(), + } +}🤖 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 `@pkg/eventservice/event_broker.go` around lines 1182 - 1199, Extract the repeated common.KeyspaceMeta construction into a helper method on dispatcherStat, using d.info.GetTableSpan().KeyspaceID and d.changefeedStat.changefeedID.Keyspace(). Update getSchemaBlockedDispatcherBucket, removeSchemaBlockedDispatcher, and getScanTaskRequestResult to call this helper instead of constructing the key inline.
1225-1242: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a dependency note for the schema frontier bucket.
GetTableDDLEventStatereturnsstore.resolvedTs.Load()for the whole keyspace and only usestableIDforMaxEventCommitTs. IfResolvedTsis later changed to table-scoped, this bucket will compare every dispatcher in the keyspace against the first dispatcher’s table frontier; add a short comment to preserve this invariant.🤖 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 `@pkg/eventservice/event_broker.go` around lines 1225 - 1242, Add a concise comment beside the GetTableDDLEventState call in the firstSchemaBlockedDispatcher bucket flow documenting that ResolvedTs is keyspace-scoped, while tableID only affects MaxEventCommitTs; preserve this invariant so the bucket does not compare dispatchers against a table-scoped frontier.maintainer/maintainer_manager.go (1)
136-151: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a floor interval for prompt heartbeats.
m.heartbeatChhas capacity 1, so it coalesces only one pending signal. EverymarkStatusChangedcall from a low-latency maintainer can therefore trigger one extrasendHeartbeatround. Under frequent status changes across many maintainers, this loop can send coordinator heartbeats far more often than the 200 ms ticker.A minimum spacing between prompt heartbeats would bound the control-message rate while keeping the latency benefit.
♻️ Sketch: gate prompt heartbeats by a floor interval
func (m *Manager) Run(ctx context.Context) error { ticker := time.NewTicker(defaultManagerHeartbeatInterval) defer ticker.Stop() + var lastPromptHeartbeat time.Time for { select { case <-ctx.Done(): return ctx.Err() case msg := <-m.msgCh: m.handleMessage(msg) case <-m.heartbeatCh: - m.sendHeartbeat() + if time.Since(lastPromptHeartbeat) >= minPromptHeartbeatInterval { + lastPromptHeartbeat = time.Now() + m.sendHeartbeat() + } case <-ticker.C:🤖 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 `@maintainer/maintainer_manager.go` around lines 136 - 151, Update the heartbeat handling in the manager loop around m.heartbeatCh and sendHeartbeat to enforce a minimum spacing between prompt heartbeats, using the existing 200 ms heartbeat interval or an appropriate floor constant. Track the last prompt heartbeat time, only send when the floor has elapsed, and preserve ticker-driven heartbeats and channel coalescing behavior.
🤖 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 `@logservice/coordinator/coordinator.go`:
- Around line 321-349: The updateChangefeedMetrics flow must not recompute or
publish the minimum during an incomplete reporting round. Gate minimum
calculation and updates to minLogServiceResolvedTs and resolvedTsGauge on
nodesReportedSinceLastUpdate reaching a complete round; otherwise, retain the
last published minimum and refresh only resolvedTsLagGauge from it. Add a test
covering reportChangefeedMetrics between partial node reports.
In `@maintainer/maintainer_manager_maintainers.go`:
- Around line 219-223: Update the newMaintainer factory and NewMaintainer
initialization so managerHeartbeatCh is assigned before NewMaintainer starts any
goroutines or installs callbacks that may invoke markStatusChanged. Prefer
passing p.heartbeatCh into NewMaintainer and setting the field during
construction, preserving the existing channel used by the manager.
In `@pkg/eventservice/event_broker_test.go`:
- Around line 76-86: Update the goroutine setup in the test’s WaitGroup flow to
use wg.Go instead of manually calling wg.Add(1), launching a goroutine, and
deferring wg.Done(). Preserve the existing start-channel synchronization and
broker.requestScan(disp) invocation.
In `@pkg/eventservice/event_broker.go`:
- Around line 1106-1108: Update the blocking enqueue in onNotify to select
between sending to c.taskChan[d.scanWorkerIndex] and broker-context
cancellation. On cancellation, call finishScan with the interrupted-shutdown
outcome so the dispatcher cannot remain in dispatcherScanQueued after close;
preserve the existing send behavior while the broker is active.
---
Nitpick comments:
In `@maintainer/maintainer_manager.go`:
- Around line 136-151: Update the heartbeat handling in the manager loop around
m.heartbeatCh and sendHeartbeat to enforce a minimum spacing between prompt
heartbeats, using the existing 200 ms heartbeat interval or an appropriate floor
constant. Track the last prompt heartbeat time, only send when the floor has
elapsed, and preserve ticker-driven heartbeats and channel coalescing behavior.
In `@pkg/eventservice/event_broker.go`:
- Around line 1182-1199: Extract the repeated common.KeyspaceMeta construction
into a helper method on dispatcherStat, using d.info.GetTableSpan().KeyspaceID
and d.changefeedStat.changefeedID.Keyspace(). Update
getSchemaBlockedDispatcherBucket, removeSchemaBlockedDispatcher, and
getScanTaskRequestResult to call this helper instead of constructing the key
inline.
- Around line 1225-1242: Add a concise comment beside the GetTableDDLEventState
call in the firstSchemaBlockedDispatcher bucket flow documenting that ResolvedTs
is keyspace-scoped, while tableID only affects MaxEventCommitTs; preserve this
invariant so the bucket does not compare dispatchers against a table-scoped
frontier.
🪄 Autofix
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 Plus
Run ID: 25b39436-5b08-4719-9c94-ea9eb8a7c330
⛔ Files ignored due to path filters (1)
eventpb/event.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (35)
api/v2/changefeed_toml_test.goapi/v2/model.goapi/v2/model_test.godownstreamadapter/dispatcher/basic_dispatcher.godownstreamadapter/dispatcher/basic_dispatcher_info.godownstreamadapter/dispatcher/event_dispatcher_test.godownstreamadapter/dispatchermanager/dispatcher_manager.godownstreamadapter/dispatchermanager/dispatcher_manager_test.godownstreamadapter/dispatchermanager/task.godownstreamadapter/dispatchermanager/task_test.godownstreamadapter/eventcollector/dispatcher_session.godownstreamadapter/eventcollector/dispatcher_stat_test.godownstreamadapter/eventcollector/event_collector_test.goeventpb/event.protologservice/coordinator/coordinator.gologservice/coordinator/coordinator_test.gologservice/eventstore/event_store.gologservice/eventstore/event_store_test.gomaintainer/maintainer.gomaintainer/maintainer_manager.gomaintainer/maintainer_manager_maintainers.gomaintainer/maintainer_manager_test.gomaintainer/maintainer_test.gopkg/config/changefeed.gopkg/config/changefeed_test.gopkg/config/replica_config.gopkg/config/replica_config_test.gopkg/eventservice/dispatcher_stat.gopkg/eventservice/event_broker.gopkg/eventservice/event_broker_test.gopkg/eventservice/event_service.gopkg/eventservice/event_service_test.gopkg/eventservice/metrics_collector.gopkg/messaging/message.gopkg/metrics/event_service.go
Publish LogCoordinator resolved lag only from complete node-report rounds and calculate it at each node's report time. Preserve Maintainer watermark ordering, expose checkpoint/resolved lag atomically, and update dashboards to use the paired metrics.
Restore the unrelated Maintainer metric collector, watermark state, and dashboard changes. Keep this follow-up scoped to LogCoordinator owner resolved-ts lag calculation and its tests.
Keep the first changefeed-level low-latency PR free of pkg/eventservice changes. Move the coupled EventStore subscription isolation and EventService dropped-task metric into the follow-up part as well so both trees compile independently.
|
/test all |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand, lidezhu 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:
|
What problem does this PR solve?
Issue Number: ref #5705
TiCDC needs a per-changefeed choice between throughput-oriented and
latency-oriented replication so both workloads can coexist on the same
captures. This PR introduces the persisted performance mode and the
control-plane behavior needed by low-latency changefeeds.
What is changed and how it works?
performance-modetoReplicaConfig, API/TOML conversion, anddispatcher register/reset messages. Throughput remains the default.
calculation/reporting only for low-latency changefeeds.
rounds and calculate lag at each node's report time.
subscriptions to Part 2, eventservice: optimize changefeed low-latency scheduling #5900.
Check List
Tests
throughput changefeeds with 100k tables each, 200k isolated subscriptions,
about 20 MB/s shared traffic for 30 minutes; both feeds remained normal)
Three-capture, 10-minute results; values are low-latency/throughput/master.
Resolved is maintainer resolved-ts lag; CPU and RSS are aggregate.
This reduces checkpoint/resolved lag by 64.0%/48.7% with +3.0% CPU and -2.4%
RSS. Table-scale latency gains cost 23-34% CPU; RSS stays within 4%.
Questions
Will it cause performance regression or break compatibility?
No compatibility break is expected. Existing configurations default to
throughput mode. Low-latency mode intentionally increases control-plane update
frequency; the mode-specific EventStore behavior is isolated in #5900.
Do you need to update user documentation, design documentation or monitoring documentation?
Yes. The new changefeed
performance-modeoption needs user documentation.The owner resolved-lag metric keeps its name and receives clarified semantics.
Release note
Summary by CodeRabbit
New Features
Improvements