sink: add async ddl timeout for add index - #5836
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
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/XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMySQL sink configuration now supports asynchronous DDL timeouts and a separate TiDB control pool. Sink construction manages the pool lifecycle. TiDB ChangesTiDB asynchronous DDL execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SinkConfig
participant TiDBSink
participant DDLWriter
participant ControlDB
participant ControlAsyncDB
SinkConfig->>TiDBSink: create DML, control, and async control pools
TiDBSink->>DDLWriter: set ControlAsyncDB
DDLWriter->>ControlDB: retrieve DDL timestamp
DDLWriter->>ControlAsyncDB: execute TiDB ADD INDEX
ControlAsyncDB-->>DDLWriter: commit transaction
Possibly related PRs
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 |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/sink/mysql/config.go`:
- Around line 285-293: Validate the config-supplied AsyncDDLTimeout before
assigning it in mergeConfig alongside the other MySQLConfig duration fields.
Ensure invalid duration strings return an error, while valid values are
converted and assigned consistently; keep Apply’s query-parameter handling and
non-TiDB behavior unchanged.
- Around line 356-389: Extract the duplicated dmlDB-then-controlDB cleanup and
warning logic from the TiDB error paths in the surrounding connection setup
function into a small helper, parameterized by the failure-context message
suffix. Replace both inline cleanup blocks after setDSNReadTimeout and
CreateMysqlDBConn failures with calls to that helper, preserving the existing
close order, warning fields, and returned errors.
🪄 Autofix (Beta)
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: 1a57d499-b353-4971-b15f-2a2e63969a86
📒 Files selected for processing (8)
downstreamadapter/sink/mysql/sink.godownstreamadapter/sink/mysql/sink_test.gopkg/config/sink.gopkg/sink/mysql/config.gopkg/sink/mysql/config_test.gopkg/sink/mysql/mysql_writer.gopkg/sink/mysql/mysql_writer_ddl.gopkg/sink/mysql/mysql_writer_test.go
| if cfg != nil && | ||
| cfg.SinkConfig != nil && | ||
| cfg.SinkConfig.MySQLConfig != nil && | ||
| cfg.SinkConfig.MySQLConfig.AsyncDDLTimeout != nil { | ||
| c.AsyncDDLTimeout = *cfg.SinkConfig.MySQLConfig.AsyncDDLTimeout | ||
| } | ||
| if err = getDuration(query, "async-ddl-timeout", &c.AsyncDDLTimeout); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'func getDuration($$$) $$$' --lang go pkg/sink/mysqlRepository: pingcap/ticdc
Length of output: 768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- config.go relevant sections ---\n'
sed -n '250,300p' pkg/sink/mysql/config.go
printf '\n--- NewMysqlConfigAndDBs / apply timeout sections ---\n'
sed -n '320,410p' pkg/sink/mysql/config.go
printf '\n--- mergeConfig / duration setters ---\n'
sed -n '170,250p' pkg/sink/mysql/config.go
printf '\n--- setDSNReadTimeout usages ---\n'
rg -n "setDSNReadTimeout|IsTiDB|AsyncDDLTimeout|DialTimeout|WriteTimeout|ReadTimeout" pkg/sink/mysql/config.goRepository: pingcap/ticdc
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- config.go relevant sections ---'
sed -n '250,300p' pkg/sink/mysql/config.go
printf '%s\n' ''
printf '%s\n' '--- NewMysqlConfigAndDBs timeout sections ---'
sed -n '320,410p' pkg/sink/mysql/config.go
printf '%s\n' ''
printf '%s\n' '--- mergeConfig and duration setters ---'
sed -n '170,250p' pkg/sink/mysql/config.go
printf '%s\n' ''
printf '%s\n' '--- setDSNReadTimeout usages ---'
rg -n "setDSNReadTimeout|IsTiDB|AsyncDDLTimeout|DialTimeout|WriteTimeout|ReadTimeout" pkg/sink/mysql/config.goRepository: pingcap/ticdc
Length of output: 9965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- setDSNReadTimeout implementation ---'
sed -n '460,500p' pkg/sink/mysql/config.go
printf '%s\n' ''
printf '%s\n' '--- Apply merge and duration helpers area ---'
sed -n '200,295p' pkg/sink/mysql/config.go
printf '%s\n' ''
printf '%s\n' '--- getDuration helper ---'
sed -n '700,735p' pkg/sink/mysql/config.goRepository: pingcap/ticdc
Length of output: 5849
Validate AsyncDDLTimeout when it is copied from config.
Apply() copies cfg.SinkConfig.MySQLConfig.AsyncDDLTimeout directly, then calls getDuration("async-ddl-timeout", ...) which only checks URI query params. setDSNReadTimeout() validates cfg.AsyncDDLTimeout only for TiDB downstreams. A config-supplied invalid duration passes Apply() for non-TiDB MySQL downstreams. Validate the config string before assigning it, ideally in mergeConfig with the other MySQLConfig duration fields.
🤖 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/sink/mysql/config.go` around lines 285 - 293, Validate the
config-supplied AsyncDDLTimeout before assigning it in mergeConfig alongside the
other MySQLConfig duration fields. Ensure invalid duration strings return an
error, while valid values are converted and assigned consistently; keep Apply’s
query-parameter handling and non-TiDB behavior unchanged.
|
/test all |
|
/test all |
1 participant
What problem does this PR solve?
Issue Number: close #5711 close #1926
What is changed and how it works?
The change adds a dedicated timeout path for TiDB async DDL, specifically ADD INDEX.
Added a new MySQL sink option
async-ddl-timeout, the default value is 10s. It can be set in sink URI:mysql://.../?async-ddl-timeout=30mCheck 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
Summary by CodeRabbit
New Features
ADD INDEXoperations now use a dedicated asynchronous connection with its own timeout.Bug Fixes