dumpling: New terminology for MySQL (#57188)#66854
dumpling: New terminology for MySQL (#57188)#66854ti-chi-bot wants to merge 10 commits intopingcap:release-8.5from
Conversation
ti-chi-bot
added
component/dumpling
|
[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 |
📝 WalkthroughWalkthroughThis pull request adds support for MySQL 8.4.0 by introducing version-aware logic to use new terminology (SHOW BINARY LOG STATUS, SHOW REPLICA STATUS) instead of deprecated SHOW MASTER STATUS and SHOW SLAVE STATUS commands. Function signatures are updated to pass ServerInfo instead of ServerType for version-aware command selection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dumpling/export/metadata_test.go`:
- Line 43: Add a real MySQL 8.4 regression test by calling recordGlobalMetaData
with a ServerInfo that includes ServerVersion: semver.New("8.4.0") (e.g.,
version.ServerInfo{ServerType: version.ServerTypeMySQL, ServerVersion:
semver.New("8.4.0")}) so the MySQL 8.4 branches run; in metadata_test.go add an
assertion that the executed query is the new "SHOW BINARY LOG STATUS" (and the
corresponding replica query "SHOW REPLICA STATUS" if applicable) and assert the
emitted metadata text matches the expected output for 8.4; apply the same change
pattern to the other test call sites you listed (lines 75-76, 111, 143, 174,
200, 242, 269-282, 306) so at least one test exercises the new MySQL 8.4
behavior and fails before the fix and passes after.
In `@dumpling/export/metadata.go`:
- Around line 68-77: The header logic in recordGlobalMetaData's
writeMasterStatusHeader currently bases the label solely on ServerVersion,
causing MariaDB (or other non-MySQL) servers with newer parsed versions to emit
"SHOW BINARY LOG STATUS:" even though dumpling/export/sql.go still runs "SHOW
MASTER STATUS"; update writeMasterStatusHeader to require serverInfo.ServerType
== version.ServerTypeMySQL in addition to the existing ServerVersion check (and
minNewTerminologyMySQL comparison) so the header only flips for MySQL
servers—modify the conditional in writeMasterStatusHeader within
recordGlobalMetaData to include that server type guard.
- Around line 176-184: The metadata writer currently hardcodes the header "SHOW
SLAVE STATUS:" for follower entries even though the earlier logic sets the
actual SQL string in the local variable query (which may be "SHOW REPLICA
STATUS" for MySQL 8.4+ or "SHOW ALL SLAVES STATUS" for MariaDB); update the
formatter that emits follower entries to use the query variable as the header
instead of the fixed string so the metadata reflects the actual command run
(locate the code that prints each follower entry and replace the hardcoded
header with the query variable).
In `@dumpling/tests/consistency/run.sh`:
- Around line 35-44: The script uses bash-only [[ ... =~ ... ]] tests on
variable versioninfo when deciding which SHOW command to run (setting metadata
via run_sql), which breaks under /bin/sh; fix by either changing the shebang to
#!/bin/bash or rewriting the checks to POSIX-compatible tests: replace the [[
$versioninfo =~ (Ti|Maria)DB ]] check with a case or a grep-based test (e.g.
case "$versioninfo" in *TiDB*|*MariaDB*) and replace the VERSION() regex test
with grep -E 'VERSION\(\): (8\.4|9)' (or an equivalent case pattern) so the
logic that calls run_sql "show master status;" or run_sql "show binary log
status;" remains identical but works under a POSIX shell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0eb89651-7837-4c57-ad22-76d60fe4f1c7
📒 Files selected for processing (7)
.github/workflows/integration-test-dumpling.ymldumpling/export/config.godumpling/export/dump.godumpling/export/metadata.godumpling/export/metadata_test.godumpling/export/sql.godumpling/tests/consistency/run.sh
|
|
||
| m := newGlobalMetadata(tcontext.Background(), createStorage(t), "") | ||
| require.NoError(t, m.recordGlobalMetaData(conn, version.ServerTypeMySQL, false)) | ||
| require.NoError(t, m.recordGlobalMetaData(conn, version.ServerInfo{ServerType: version.ServerTypeMySQL}, false)) |
There was a problem hiding this comment.
Add a real MySQL 8.4 regression test.
All of these updated call sites still pass version.ServerInfo{ServerType: ...} with ServerVersion == nil, so recordGlobalMetaData() only exercises the legacy SHOW MASTER STATUS / SHOW SLAVE STATUS branches. That means this PR can still pass without covering the new MySQL 8.4 behavior (SHOW BINARY LOG STATUS and SHOW REPLICA STATUS). Please add at least one case with ServerVersion: semver.New("8.4.0") and assert both the executed query and the emitted metadata text.
As per coding guidelines MUST add a regression test and verify it fails before fix and passes after fix for bug fixes.
Also applies to: 75-76, 111-111, 143-143, 174-174, 200-200, 242-242, 269-282, 306-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dumpling/export/metadata_test.go` at line 43, Add a real MySQL 8.4 regression
test by calling recordGlobalMetaData with a ServerInfo that includes
ServerVersion: semver.New("8.4.0") (e.g., version.ServerInfo{ServerType:
version.ServerTypeMySQL, ServerVersion: semver.New("8.4.0")}) so the MySQL 8.4
branches run; in metadata_test.go add an assertion that the executed query is
the new "SHOW BINARY LOG STATUS" (and the corresponding replica query "SHOW
REPLICA STATUS" if applicable) and assert the emitted metadata text matches the
expected output for 8.4; apply the same change pattern to the other test call
sites you listed (lines 75-76, 111, 143, 174, 200, 242, 269-282, 306) so at
least one test exercises the new MySQL 8.4 behavior and fails before the fix and
passes after.
| func recordGlobalMetaData(tctx *tcontext.Context, db *sql.Conn, buffer *bytes.Buffer, serverInfo version.ServerInfo, afterConn bool, snapshot string) error { // revive:disable-line:flag-parameter | ||
| serverType := serverInfo.ServerType | ||
| writeMasterStatusHeader := func() { | ||
| buffer.WriteString("SHOW MASTER STATUS:") | ||
| if serverInfo.ServerVersion == nil { | ||
| buffer.WriteString("SHOW MASTER STATUS:") | ||
| } else if serverInfo.ServerVersion.LessThan(*minNewTerminologyMySQL) { | ||
| buffer.WriteString("SHOW MASTER STATUS:") | ||
| } else { | ||
| buffer.WriteString("SHOW BINARY LOG STATUS:") | ||
| } |
There was a problem hiding this comment.
Only flip the header for MySQL.
writeMasterStatusHeader() now keys off ServerVersion alone, so MariaDB 11.x will emit SHOW BINARY LOG STATUS: even though dumpling/export/sql.go still executes SHOW MASTER STATUS for MariaDB. The same drift can happen for any non-MySQL server whose parsed version crosses 8.4.0. Gate this on serverInfo.ServerType == version.ServerTypeMySQL too.
Proposed fix
writeMasterStatusHeader := func() {
- if serverInfo.ServerVersion == nil {
- buffer.WriteString("SHOW MASTER STATUS:")
- } else if serverInfo.ServerVersion.LessThan(*minNewTerminologyMySQL) {
- buffer.WriteString("SHOW MASTER STATUS:")
- } else {
+ if serverInfo.ServerType == version.ServerTypeMySQL &&
+ serverInfo.ServerVersion != nil &&
+ !serverInfo.ServerVersion.LessThan(*minNewTerminologyMySQL) {
buffer.WriteString("SHOW BINARY LOG STATUS:")
+ } else {
+ buffer.WriteString("SHOW MASTER STATUS:")
}
if afterConn {
buffer.WriteString(" /* AFTER CONNECTION POOL ESTABLISHED */")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dumpling/export/metadata.go` around lines 68 - 77, The header logic in
recordGlobalMetaData's writeMasterStatusHeader currently bases the label solely
on ServerVersion, causing MariaDB (or other non-MySQL) servers with newer parsed
versions to emit "SHOW BINARY LOG STATUS:" even though dumpling/export/sql.go
still runs "SHOW MASTER STATUS"; update writeMasterStatusHeader to require
serverInfo.ServerType == version.ServerTypeMySQL in addition to the existing
ServerVersion check (and minNewTerminologyMySQL comparison) so the header only
flips for MySQL servers—modify the conditional in writeMasterStatusHeader within
recordGlobalMetaData to include that server type guard.
| if isms { | ||
| query = "SHOW ALL SLAVES STATUS" | ||
| query = "SHOW ALL SLAVES STATUS" // MariaDB | ||
| } else if serverInfo.ServerVersion == nil { | ||
| query = "SHOW SLAVE STATUS" // Unknown version | ||
| } else if serverInfo.ServerType == version.ServerTypeMySQL && | ||
| !serverInfo.ServerVersion.LessThan(*minNewTerminologyMySQL) { | ||
| query = "SHOW REPLICA STATUS" // MySQL 8.4.0 and newer | ||
| } else { | ||
| query = "SHOW SLAVE STATUS" | ||
| query = "SHOW SLAVE STATUS" // MySQL |
There was a problem hiding this comment.
Use the selected follower-status command in the metadata output.
This branch now chooses SHOW REPLICA STATUS for MySQL 8.4+, but the formatter below still hardcodes SHOW SLAVE STATUS: before every follower entry. The metadata file will therefore advertise the wrong statement even when the query succeeded. Please derive that header from query instead of a fixed string.
Proposed fix
- if len(host) > 0 {
- buffer.WriteString("SHOW SLAVE STATUS:\n")
+ if len(host) > 0 {
+ buffer.WriteString(query + ":\n")
if isms {
buffer.WriteString("\tConnection name: " + connName + "\n")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dumpling/export/metadata.go` around lines 176 - 184, The metadata writer
currently hardcodes the header "SHOW SLAVE STATUS:" for follower entries even
though the earlier logic sets the actual SQL string in the local variable query
(which may be "SHOW REPLICA STATUS" for MySQL 8.4+ or "SHOW ALL SLAVES STATUS"
for MariaDB); update the formatter that emits follower entries to use the query
variable as the header instead of the fixed string so the metadata reflects the
actual command run (locate the code that prints each follower entry and replace
the hardcoded header with the query variable).
| if [[ $versioninfo =~ (Ti|Maria)DB ]]; then | ||
| metadata=`run_sql "show master status;"` | ||
| else | ||
| if [[ $versioninfo =~ "VERSION(): "(8.4|9) ]]; then | ||
| # MySQL 8.4.0 and newer no longer support SHOW MASTER STATUS | ||
| # and only support SHOW BINARY LOG STATUS | ||
| metadata=`run_sql "show binary log status;"` | ||
| else | ||
| metadata=`run_sql "show master status;"` | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1p;35,44p' dumpling/tests/consistency/run.sh
python - <<'PY'
from pathlib import Path
lines = Path("dumpling/tests/consistency/run.sh").read_text().splitlines()
print("shebang:", lines[0])
for i, line in enumerate(lines, 1):
if "[[" in line or "=~" in line:
print(f"{i}: {line}")
PYRepository: pingcap/tidb
Length of output: 540
Fix POSIX compatibility or change shebang to bash.
The script declares #!/bin/sh but uses bash-only [[ ... =~ ... ]] syntax on lines 35 and 38. This causes a parsing error on POSIX-compliant shells like dash or ash, preventing the consistency test from running at all. Either rewrite the version checks using POSIX case/grep, or change the shebang to #!/bin/bash if bash is guaranteed in the test environment.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 35-35: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 35-35: In POSIX sh, =~ regex matching is undefined.
(SC3015)
[warning] 38-38: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 38-38: In POSIX sh, =~ regex matching is undefined.
(SC3015)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dumpling/tests/consistency/run.sh` around lines 35 - 44, The script uses
bash-only [[ ... =~ ... ]] tests on variable versioninfo when deciding which
SHOW command to run (setting metadata via run_sql), which breaks under /bin/sh;
fix by either changing the shebang to #!/bin/bash or rewriting the checks to
POSIX-compatible tests: replace the [[ $versioninfo =~ (Ti|Maria)DB ]] check
with a case or a grep-based test (e.g. case "$versioninfo" in *TiDB*|*MariaDB*)
and replace the VERSION() regex test with grep -E 'VERSION\(\): (8\.4|9)' (or an
equivalent case pattern) so the logic that calls run_sql "show master status;"
or run_sql "show binary log status;" remains identical but works under a POSIX
shell.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #66854 +/- ##
================================================
Coverage ? 55.0111%
================================================
Files ? 1819
Lines ? 652532
Branches ? 0
================================================
Hits ? 358965
Misses ? 266928
Partials ? 26639
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@ti-chi-bot: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
We already have #65131 for this |
ti-chi-bot
bot
added
cherry-pick-approved
2 participants
This is an automated cherry-pick of #57188
What problem does this PR solve?
Issue Number: close #53082
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Tests