dumpling: Fix and improve MySQL 8.4 support#66704
dumpling: Fix and improve MySQL 8.4 support#66704dveeden wants to merge 1 commit intopingcap:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMade ServerTypeUnknown a typed constant, standardized master-status header text, broadened replica column-name mappings, reorganized metadata tests to be version-aware with added per-version cases, and updated the CI MySQL version matrix. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
component/dumpling
|
Hi @dveeden. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
ti-chi-bot
bot
added
size/L
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dumpling/export/metadata_test.go (1)
261-261: Make the MariaDB “no GTID” behavior explicit in mocks.This test intends a no-GTID scenario, but it currently relies on implicit fallback. Add an explicit expectation for
SELECT @@global.gtid_binlog_posreturning an error.Proposed refactor
mock.ExpectQuery("SHOW MASTER STATUS").WillReturnRows(rows) + mock.ExpectQuery("SELECT @@global.gtid_binlog_pos"). + WillReturnError(errors.New("gtid_binlog_pos unavailable")) mock.ExpectQuery("SELECT @@default_master_connection"). WillReturnRows(sqlmock.NewRows([]string{"@@default_master_connection"}). AddRow("connection_1"))Based on learnings: Applies to /{*_test.go,testdata/} : Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.
Also applies to: 277-281
🤖 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 261, The test relies on implicit fallback for a MariaDB "no GTID" scenario; update the test's mock expectations to explicitly simulate that SELECT @@global.gtid_binlog_pos fails by adding a mock expectation for that query to return an error (e.g., with mock.ExpectQuery("SELECT @@global.gtid_binlog_pos").WillReturnError(...)). Apply the same explicit expectation in the related assertions around lines referenced (the other MariaDB/no-GTID case), so the test deterministically models the no-GTID behavior instead of relying on implicit fallback.
🤖 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 183: The test's SQL expectation string in mock.ExpectQuery currently has
a double space ("SHOW BINARY LOG STATUS") which doesn't match the MySQL 8.4
emitted query ("SHOW BINARY LOG STATUS"); update the expectation in
dumpling/export/metadata_test.go by changing the mock.ExpectQuery call that
references "SHOW BINARY LOG STATUS" to use the single-space exact string "SHOW
BINARY LOG STATUS" so WillReturnRows(rows) matches the actual query.
---
Nitpick comments:
In `@dumpling/export/metadata_test.go`:
- Line 261: The test relies on implicit fallback for a MariaDB "no GTID"
scenario; update the test's mock expectations to explicitly simulate that SELECT
@@global.gtid_binlog_pos fails by adding a mock expectation for that query to
return an error (e.g., with mock.ExpectQuery("SELECT
@@global.gtid_binlog_pos").WillReturnError(...)). Apply the same explicit
expectation in the related assertions around lines referenced (the other
MariaDB/no-GTID case), so the test deterministically models the no-GTID behavior
instead of relying on implicit fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: af73fe74-2885-418e-8df7-2651fe9180ee
📒 Files selected for processing (3)
br/pkg/version/version.godumpling/export/metadata.godumpling/export/metadata_test.go
|
Review Failed Environment preparation failed after 3 attempts. The sandbox infrastructure (upstream AI provider) returned 503 Service Unavailable errors on all attempts. Please retry the review when the infrastructure recovers. ℹ️ Learn more details on Pantheon AI. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
dumpling/export/metadata_test.go (3)
256-256: Remove unnecessary type cast for consistency.
version.ServerTypeMariaDBis already of typeServerType. Other tests in this file (e.g., line 285) don't use the cast. Consider removing for consistency.Suggested fix
- require.Equal(t, version.ServerType(version.ServerTypeMariaDB), si.ServerType) + require.Equal(t, version.ServerTypeMariaDB, si.ServerType)🤖 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 256, The test assertion unnecessarily casts version.ServerTypeMariaDB to version.ServerType; update the require.Equal call to compare version.ServerTypeMariaDB directly to si.ServerType (remove the explicit type conversion) so it matches other tests like the one referencing version.ServerTypeMariaDB later and keeps consistency in the metadata_test.go assertions.
398-398: Remove unnecessary type cast for consistency.Same pattern — remove the redundant cast.
Suggested fix
- require.Equal(t, version.ServerType(version.ServerTypeTiDB), si.ServerType) + require.Equal(t, version.ServerTypeTiDB, si.ServerType)🤖 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 398, In require.Equal(t, version.ServerType(version.ServerTypeTiDB), si.ServerType) remove the redundant cast by passing the enum constant directly; update the assertion to compare version.ServerTypeTiDB with si.ServerType so the test uses the constant directly (referencing require.Equal, version.ServerTypeTiDB, and si.ServerType).
359-359: Remove unnecessary type cast for consistency.Same as above —
version.ServerTypeTiDBis already of typeServerType.Suggested fix
- require.Equal(t, version.ServerType(version.ServerTypeTiDB), si.ServerType) + require.Equal(t, version.ServerTypeTiDB, si.ServerType)🤖 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 359, The test assertion unnecessarily casts version.ServerTypeTiDB to version.ServerType; update the require.Equal call in the test to compare version.ServerTypeTiDB directly to si.ServerType by removing the redundant version.ServerType(...) cast so the line reads require.Equal(t, version.ServerTypeTiDB, si.ServerType) and keeps types consistent with the other assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dumpling/export/metadata_test.go`:
- Line 256: The test assertion unnecessarily casts version.ServerTypeMariaDB to
version.ServerType; update the require.Equal call to compare
version.ServerTypeMariaDB directly to si.ServerType (remove the explicit type
conversion) so it matches other tests like the one referencing
version.ServerTypeMariaDB later and keeps consistency in the metadata_test.go
assertions.
- Line 398: In require.Equal(t, version.ServerType(version.ServerTypeTiDB),
si.ServerType) remove the redundant cast by passing the enum constant directly;
update the assertion to compare version.ServerTypeTiDB with si.ServerType so the
test uses the constant directly (referencing require.Equal,
version.ServerTypeTiDB, and si.ServerType).
- Line 359: The test assertion unnecessarily casts version.ServerTypeTiDB to
version.ServerType; update the require.Equal call in the test to compare
version.ServerTypeTiDB directly to si.ServerType by removing the redundant
version.ServerType(...) cast so the line reads require.Equal(t,
version.ServerTypeTiDB, si.ServerType) and keeps types consistent with the other
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 60e384fe-fbb0-49fb-b430-009e4f5df091
📒 Files selected for processing (1)
dumpling/export/metadata_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66704 +/- ##
================================================
- Coverage 77.6754% 77.0933% -0.5822%
================================================
Files 2009 1930 -79
Lines 550314 538105 -12209
================================================
- Hits 427459 414843 -12616
- Misses 121147 123255 +2108
+ Partials 1708 7 -1701
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Review Failed Environment preparation failed after 3 attempts. The sandbox infrastructure (upstream AI provider CR2) is returning 503 Service Unavailable errors on all attempts. Please retry the review once the infrastructure recovers. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
the
needs-cherry-pick-release-8.5
dveeden
force-pushed
the
dumpling_mysql84_202603
branch
from
b4b9cda to
3305f34
Compare
|
[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 |
dveeden
force-pushed
the
dumpling_mysql84_202603
branch
2 times, most recently
from
47f0ce6 to
69f090e
Compare
dveeden
removed
the
needs-cherry-pick-release-8.5
ti-chi-bot
bot
added
the
needs-cherry-pick-release-8.5
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dumpling/export/metadata_test.go (1)
261-306:⚠️ Potential issue | 🟠 MajorModel the MariaDB GTID lookup in these follower tests.
recordGlobalMetaDatastill callsSELECT @@global.gtid_binlog_posfor MariaDB before it inspects follower status (dumpling/export/metadata.go:131-146). These fixtures skip that query and hard-code an empty master GTID line, so they are not asserting the current MariaDB path. Please either add the GTID expectation here and updateexpected, or change the production code if the new contract is to omit MariaDB GTID entirely.As per coding guidelines: "`**/*_test.go`: MUST add a regression test and verify it fails before fix and passes after fix for bug fixes."🧪 Example adjustment
mock.ExpectQuery("SHOW MASTER STATUS").WillReturnRows(rows) +mock.ExpectQuery("SELECT @@global.gtid_binlog_pos"). + WillReturnRows(sqlmock.NewRows([]string{"@@global.gtid_binlog_pos"}).AddRow("")) mock.ExpectQuery("SELECT @@default_master_connection"). WillReturnRows(sqlmock.NewRows([]string{"@@default_master_connection"}). AddRow("connection_1"))mock.ExpectQuery("SHOW MASTER STATUS").WillReturnRows(rows) +mock.ExpectQuery("SELECT @@global.gtid_binlog_pos"). + WillReturnRows(sqlmock.NewRows([]string{"@@global.gtid_binlog_pos"}).AddRow("0-1-2")) mock.ExpectQuery("SELECT @@default_master_connection"). WillReturnRows(sqlmock.NewRows([]string{"@@default_master_connection"}). AddRow("connection_1")) ... - "\tGTID:\n\n" + + "\tGTID:0-1-2\n\n" +Also applies to: 308-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dumpling/export/metadata_test.go` around lines 261 - 306, The tests in metadata_test.go are missing the MariaDB GTID lookup that recordGlobalMetaData invokes (the SELECT @@global.gtid_binlog_pos query), so add a mock ExpectQuery for "SELECT @@global.gtid_binlog_pos" returning a row with the GTID value you want represented, and then update the expected string(s) (the expected variable in TestMariaDBWithFollowersMetaData_File and the similar test around lines 308-357) to include that GTID line under the appropriate SHOW MASTER/SHOW SLAVE sections; reference recordGlobalMetaData and the SELECT @@global.gtid_binlog_pos query so the fixtures match production behavior.
🧹 Nitpick comments (1)
dumpling/export/metadata.go (1)
71-77: Explain why the metadata label staysSHOW MASTER STATUS:.
ShowMasterStatusindumpling/export/sql.goswitches toSHOW BINARY LOG STATUSfor MySQL 8.4+, so this fixed header is now a non-obvious compatibility choice. A short comment here would prevent future readers from “correcting” the label and accidentally changing the metadata-file contract.As per coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs, and SHOULD NOT restate what the code already makes clear."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dumpling/export/metadata.go` around lines 71 - 77, The fixed metadata header "SHOW MASTER STATUS:" in writeMasterStatusHeader is intentional and must remain unchanged to preserve the metadata-file contract even though ShowMasterStatus (in dumpling/export/sql.go) may emit "SHOW BINARY LOG STATUS" for MySQL 8.4+; add a short comment immediately above (or inside) writeMasterStatusHeader that states this invariant and references ShowMasterStatus so future readers know the label is fixed for compatibility/metadata-file reasons and should not be altered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@dumpling/export/metadata_test.go`:
- Around line 261-306: The tests in metadata_test.go are missing the MariaDB
GTID lookup that recordGlobalMetaData invokes (the SELECT
@@global.gtid_binlog_pos query), so add a mock ExpectQuery for "SELECT
@@global.gtid_binlog_pos" returning a row with the GTID value you want
represented, and then update the expected string(s) (the expected variable in
TestMariaDBWithFollowersMetaData_File and the similar test around lines 308-357)
to include that GTID line under the appropriate SHOW MASTER/SHOW SLAVE sections;
reference recordGlobalMetaData and the SELECT @@global.gtid_binlog_pos query so
the fixtures match production behavior.
---
Nitpick comments:
In `@dumpling/export/metadata.go`:
- Around line 71-77: The fixed metadata header "SHOW MASTER STATUS:" in
writeMasterStatusHeader is intentional and must remain unchanged to preserve the
metadata-file contract even though ShowMasterStatus (in dumpling/export/sql.go)
may emit "SHOW BINARY LOG STATUS" for MySQL 8.4+; add a short comment
immediately above (or inside) writeMasterStatusHeader that states this invariant
and references ShowMasterStatus so future readers know the label is fixed for
compatibility/metadata-file reasons and should not be altered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4efdee28-de99-4f57-be5d-c604901c91b8
📒 Files selected for processing (6)
.github/workflows/integration-test-dumpling.ymlbr/pkg/version/version.godumpling/export/dump_test.godumpling/export/metadata.godumpling/export/metadata_test.godumpling/export/sql_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- dumpling/export/sql_test.go
- dumpling/export/dump_test.go
dveeden
force-pushed
the
dumpling_mysql84_202603
branch
2 times, most recently
from
e4d0078 to
e6bbee3
Compare
dveeden
force-pushed
the
dumpling_mysql84_202603
branch
from
e6bbee3 to
c0ad9f5
Compare
|
@dveeden: 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. |
|
@dveeden: The following tests 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. |
1 participant
What problem does this PR solve?
Issue Number: ref #53082
Problem Summary:
What changed and how does it work?
Gtid_IO_Pos)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
Chores