tests: fix weekly random DDL single stability#5417
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
[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 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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/XXL
![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 robust random DDL/DML test runner alongside integration tests to address TiCDC replication stalls and timeouts, while updating core components (such as BasicDispatcher, BarrierEvent, SpanController, and EventScanner) to handle obsolete events, dispatcher recreations, and deleted tables correctly. The review feedback highlights several critical areas for improvement in the test runner and controller, including a potential memory leak in removedTableCheckpointTs, file descriptor leaks in the log scanner, inefficient autotune scaling, a data race on tbl.name, and a failure to restore the global tidb_enable_external_ts_read variable on downstream TiDB during syncpoint diff failures.
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.
| // removedTableCheckpointTs records the highest checkpoint reported by removed dispatchers for each table. | ||
| // It prevents a stale add-table barrier from recreating a dispatcher below events already flushed by | ||
| // the previous dispatcher of the same physical table. | ||
| removedTableCheckpointTs map[int64]uint64 |
There was a problem hiding this comment.
The removedTableCheckpointTs map keeps track of the highest checkpoint reported by removed dispatchers for each table. However, since table IDs are unique and monotonically increasing, entries are never removed from this map. In a long-running changefeed with frequent table creations and drops (e.g., partition tables), this will lead to an unbounded memory leak.
Recommendation:
Since any newly created dispatcher will have a startTs greater than or equal to the global checkpoint, any entry in removedTableCheckpointTs where the recorded checkpoint is less than or equal to the current global checkpoint can be safely garbage collected. Consider implementing a periodic cleanup mechanism to purge obsolete entries from this map.
|
|
||
| for _, path := range files { | ||
| f, err := os.Open(path) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Opening files inside a loop without a guaranteed cleanup mechanism (like defer) is risky. If an error occurs or the loop body is modified to include early returns, the file descriptor may leak.
To safely use defer inside a loop without accumulating open file descriptors, wrap the loop body's file processing in an immediately-invoked function literal.
| if sinceAdvance >= soft || successRate < 0.10 { | ||
| if nextDDL > 1 { | ||
| nextDDL-- | ||
| return autoTuneResult{nextDML: nextDML, nextDDL: nextDDL} | ||
| } | ||
| if nextDML > 1 { | ||
| nextDML -= 8 | ||
| if nextDML < 1 { | ||
| nextDML = 1 | ||
| } | ||
| } | ||
| return autoTuneResult{nextDML: nextDML, nextDDL: nextDDL} | ||
| } |
There was a problem hiding this comment.
When a stall is detected (sinceAdvance >= soft or successRate < 0.10), the autotune logic decrements nextDDL by 1 per step and returns immediately. It does not start reducing nextDML until nextDDL has been reduced to 1.
If nextDDL is high (e.g., 8), it will take 7 steps (70 seconds with a 10-second health interval) to reduce nextDDL to 1 before nextDML is even touched. During this time, the high DML concurrency (e.g., 128) will continue to overwhelm the system, wasting valuable time and potentially triggering the hard timeout (5 minutes) and failing the test.
Recommendation:
Consider reducing both DDL and DML concurrency simultaneously when a stall is detected, or scaling down more aggressively if the stall duration increases.
| rowID := startID + int64(i) | ||
| for _, c := range cols { | ||
| args = append(args, buildRandomValue(rand.New(rand.NewSource(rowID)), tbl, c, rowID)) | ||
| } | ||
| } |
There was a problem hiding this comment.
In runBigInsertTxn, buildRandomValue is called without holding tbl.mu. Inside buildRandomValue, tbl.name is read concurrently:
case "JSON":
return fmt.Sprintf("{\"id\":%d,\"tbl\":\"%s\"}", rowID, tbl.name)At the same time, a DDL worker executing rename_table can modify tbl.name under tbl.mu.Lock(). Even though rename_table currently only targets domainChurn tables and runBigInsertTxn targets domainSplit tables, relying on domain disjointness to prevent data races is extremely fragile and prone to future regression.
Recommendation:
Retrieve and copy tbl.name (and tbl.fqName()) into local variables while holding tbl.mu at the beginning of runBigInsertTxn, and pass the copied name to buildRandomValue or lock tbl.mu inside buildRandomValue when accessing mutable fields.
| original, err := queryGlobalExternalTSRead(ctx, downstream) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if err := setGlobalExternalTSRead(ctx, downstream, "OFF"); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| diag, runErr := runSyncDiffInspector(ctx, confPath, logPath, retries) | ||
| if original == "OFF" { | ||
| return diag, runErr | ||
| } | ||
|
|
||
| restoreCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second) | ||
| restoreErr := setGlobalExternalTSRead(restoreCtx, downstream, original) | ||
| cancel() | ||
| if restoreErr != nil { | ||
| r.logger.Printf("syncpoint diff: failed to restore %s=%s: err=%v", tidbEnableExternalTSReadVar, original, restoreErr) | ||
| if runErr == nil { | ||
| return diag, restoreErr | ||
| } | ||
| } | ||
| return diag, runErr |
There was a problem hiding this comment.
If runSyncDiffInspector panics or if there is an early return, the global variable tidb_enable_external_ts_read will not be restored to its original value on the downstream TiDB. This can leave the downstream cluster in an inconsistent state and affect subsequent tests.
Recommendation:
Use defer to guarantee that the global variable is always restored when the function exits.
original, err := queryGlobalExternalTSRead(ctx, downstream)
if err != nil {
return "", err
}
if err := setGlobalExternalTSRead(ctx, downstream, "OFF"); err != nil {
return "", err
}
defer func() {
if original != "OFF" {
restoreCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
if err := setGlobalExternalTSRead(restoreCtx, downstream, original); err != nil {
r.logger.Printf("syncpoint diff: failed to restore %s=%s: err=%v", tidbEnableExternalTSReadVar, original, err)
}
}
}()
return runSyncDiffInspector(ctx, confPath, logPath, retries)Signed-off-by: dongmen <414110582@qq.com>
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
1 participant
What problem does this PR solve?
Issue Number: ref #5040
Weekly random DDL single runs need reproducible coverage and stricter failure detection so dispatcher progress regressions are actionable. The runtime replay fixes in
downstreamadapterandpkg/eventservicehave been split out to #5422.What is changed and how it works?
This PR keeps the maintainer-side barrier/operator/span changes and adds weekly random DDL runner coverage, MySQL suite entries for single, multi, failover, and slow lossy DDL cases, plus random DDL generator/log scanner tests.
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No. The remaining changes are maintainer-side bookkeeping and test coverage.
Do you need to update user documentation, design documentation or monitoring documentation?
No user documentation update is needed.
Release note