dumpling: New terminology for MySQL (#57188)#65131
dumpling: New terminology for MySQL (#57188)#65131dveeden wants to merge 6 commits intopingcap:release-8.5from
Conversation
ti-chi-bot
bot
added
release-note
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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
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. |
|
/ok-to-test |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #65131 +/- ##
================================================
Coverage ? 54.9752%
================================================
Files ? 1819
Lines ? 653531
Branches ? 0
================================================
Hits ? 359280
Misses ? 267630
Partials ? 26621
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
OliverS929
left a comment
OliverS929
left a comment
There was a problem hiding this comment.
Please take a look at problems mentioned in comments.
| } | ||
|
|
||
| func (m *globalMetadata) recordGlobalMetaData(db *sql.Conn, serverType version.ServerType, afterConn bool) error { // revive:disable-line:flag-parameter | ||
| func (m *globalMetadata) recordGlobalMetaData(db *sql.Conn, serverInfo version.ServerInfo, afterConn bool) error { // revive:disable-line:flag-parameter |
There was a problem hiding this comment.
When recordGlobalMetaData switches MySQL 8.4+ to SHOW REPLICA STATUS, it looks like the row parser still only checks the legacy column names (e.g. Master_Host, Relay_Master_Log_File, Exec_Master_Log_Pos, incl. lower-case variants). On MySQL 8.4, SHOW REPLICA STATUS returns the renamed Source_* fields (e.g. Source_Host, Relay_Source_Log_File, Exec_Source_Log_Pos), so Dumpling might not pick up replica metadata on 8.4+ even though the query succeeds.
Refs:
- MySQL 8.4 docs for
SHOW REPLICA STATUSshow theSource_*/Exec_Source_Log_Posnaming in the output fields: https://dev.mysql.com/doc/refman/8.4/en/show-replica-status.html - The current parser logic seems to be looking for the legacy names here: https://github.com/pingcap/tidb/pull/65131/changes#diff-e12d118a03ab0dd4a927373cccdf198aea6dd7f0792115bb02a1c858d0ca2e4dR200-R213
Suggestion: we could consider accepting both Master_* and Source_* column names in the parser to keep it compatible across MySQL 8.0.x and 8.4+.
There was a problem hiding this comment.
Dumpling might not pick up replica metadata on 8.4+ even though the query succeeds.
It does not pickup the metadata.
- The link you provided doesn't work. This is not a problem as as was able to easily find the piece of source code you were referring to.
- I think this can easily be fixed with the patch below.
- The tests in
metadata_test.goalso need to be extended to cover this. - Should I push this change to this PR, or should I first fix it in
master?
diff --git a/dumpling/export/metadata.go b/dumpling/export/metadata.go
index 15e26e41be..d51d087fc0 100644
--- a/dumpling/export/metadata.go
+++ b/dumpling/export/metadata.go
@@ -203,11 +203,11 @@ func recordGlobalMetaData(tctx *tcontext.Context, db *sql.Conn, buffer *bytes.Bu
switch col {
case "connection_name":
connName = data[i].String
- case "exec_master_log_pos":
+ case "exec_master_log_pos", "exec_source_log_pos":
pos = data[i].String
case "relay_master_log_file":
logFile = data[i].String
- case "master_host":
+ case "master_host", "source_host":
host = data[i].String
case "executed_gtid_set":
gtidSet = data[i].StringThere was a problem hiding this comment.
Yeah, we can do it the way you suggested — please go ahead and extend the tests in metadata_test.go to cover this as well.
Since this cherry-pick PR was raised manually and the issue was discovered in the context of this PR (not in the original PR), I think it makes sense to push the fix here first so this PR isn’t left in a flawed state.
That said, please also open a follow-up PR to apply the same change to the master branch, and link it back here for tracking/information sharing.
Just my personal take, and it applies to all my comments in this PR.
dumpling/export/metadata_test.go
Outdated
|
|
||
| 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.
The updated tests seem to construct ServerInfo without setting a version, so they may not catch any version-dependent header/statement changes. In production, ParseServerInfo typically populates ServerVersion.
Maybe consider updating the tests to set both ServerType and ServerVersion (or build ServerInfo the same way as production parsing), and add coverage for cases
There was a problem hiding this comment.
I think this might be a good fix for this:
diff --git a/dumpling/export/metadata_test.go b/dumpling/export/metadata_test.go
index e499fa2333..09b7928633 100644
--- a/dumpling/export/metadata_test.go
+++ b/dumpling/export/metadata_test.go
@@ -40,7 +40,9 @@ func TestMysqlMetaData(t *testing.T) {
sqlmock.NewRows([]string{"exec_master_log_pos", "relay_master_log_file", "master_host", "Executed_Gtid_Set", "Seconds_Behind_Master"}))
m := newGlobalMetadata(tcontext.Background(), createStorage(t), "")
- require.NoError(t, m.recordGlobalMetaData(conn, version.ServerInfo{ServerType: version.ServerTypeMySQL}, false))
+ si := version.ParseServerInfo("8.0.45")
+ require.Equal(t, version.ServerType(version.ServerTypeMySQL), si.ServerType)
+ require.NoError(t, m.recordGlobalMetaData(conn, si, false))
expected := "SHOW MASTER STATUS:\n" +
"\tLog: ON.000001\n" +Things to note:
- The type of
version.ServerTypeMySQLis int, notServerType. This should probably be fixed. - I think this and other tests could use a variant for MySQL 8.4 that has a mock with the new behavior.
If we change the type like this:
diff --git a/br/pkg/version/version.go b/br/pkg/version/version.go
index 1ab3391fab..16ed26c5ce 100644
--- a/br/pkg/version/version.go
+++ b/br/pkg/version/version.go
@@ -352,7 +352,7 @@ type ServerType int
const (
// ServerTypeUnknown represents unknown server type
- ServerTypeUnknown = iota
+ ServerTypeUnknown ServerType = iota
// ServerTypeMySQL represents MySQL server type
ServerTypeMySQL
// ServerTypeMariaDB represents MariaDB server typeThen the patch would be this:
diff --git a/dumpling/export/metadata_test.go b/dumpling/export/metadata_test.go
index e499fa2333..3ce51393bf 100644
--- a/dumpling/export/metadata_test.go
+++ b/dumpling/export/metadata_test.go
@@ -40,7 +40,9 @@ func TestMysqlMetaData(t *testing.T) {
sqlmock.NewRows([]string{"exec_master_log_pos", "relay_master_log_file", "master_host", "Executed_Gtid_Set", "Seconds_Behind_Master"}))
m := newGlobalMetadata(tcontext.Background(), createStorage(t), "")
- require.NoError(t, m.recordGlobalMetaData(conn, version.ServerInfo{ServerType: version.ServerTypeMySQL}, false))
+ si := version.ParseServerInfo("8.0.45")
+ require.Equal(t, version.ServerTypeMySQL, si.ServerType)
+ require.NoError(t, m.recordGlobalMetaData(conn, si, false))
expected := "SHOW MASTER STATUS:\n" +
"\tLog: ON.000001\n" +There was a problem hiding this comment.
Given this is a cherry-pick PR, I’d lean toward keeping the fix minimal and scoped to what’s needed for the cppr.
Also, the issue I mentioned likely applies to the other newly added test cases in this PR as well (if I recall correctly). Could you please take a look at those and adjust them in the same way where needed?
dumpling/export/metadata.go
Outdated
| serverType := serverInfo.ServerType | ||
| writeMasterStatusHeader := func() { | ||
| buffer.WriteString("SHOW MASTER STATUS:") | ||
| if serverInfo.ServerVersion == nil { |
There was a problem hiding this comment.
The new header/statement selection only compares ServerVersion >= 8.4.0 and doesn’t check ServerType. That means any TiDB/MariaDB build whose reported version compares >= 8.4 could emit a SHOW BINARY LOG STATUS: header in metadata even though Dumpling still executes SHOW MASTER STATUS for those servers. This could change metadata output for existing TiDB/MariaDB deployments unexpectedly.
There was a problem hiding this comment.
I verified this and with MariaDB 11.8 this does happen indeed.
There are two ways to fix this:
Option 1
This keeps the line the same as the command that was executed and fixes the issue you found.
diff --git a/dumpling/export/metadata.go b/dumpling/export/metadata.go
index d51d087fc0..3d804df1b2 100644
--- a/dumpling/export/metadata.go
+++ b/dumpling/export/metadata.go
@@ -68,7 +68,9 @@ func (m *globalMetadata) recordGlobalMetaData(db *sql.Conn, serverInfo version.S
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() {
- if serverInfo.ServerVersion == nil {
+ if serverInfo.ServerType != version.ServerTypeMySQL {
+ buffer.WriteString("SHOW MASTER STATUS:")
+ } else if serverInfo.ServerVersion == nil {
buffer.WriteString("SHOW MASTER STATUS:")
} else if serverInfo.ServerVersion.LessThan(*minNewTerminologyMySQL) {
buffer.WriteString("SHOW MASTER STATUS:")Note: some of the if branches could be combined, but not sure this makes a real difference.
Option 2
This always uses the same line in the output, regardless of the command that was used. This stays closer to the old mydumper format as described on https://mydumper.github.io/mydumper/docs/html/files.html#metadata
I think this is my preferred option.
diff --git a/dumpling/export/metadata.go b/dumpling/export/metadata.go
index d51d087fc0..f8a6c851ff 100644
--- a/dumpling/export/metadata.go
+++ b/dumpling/export/metadata.go
@@ -68,13 +68,7 @@ func (m *globalMetadata) recordGlobalMetaData(db *sql.Conn, serverInfo version.S
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() {
- 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:")
- }
+ buffer.WriteString("SHOW MASTER STATUS:")
if afterConn {
buffer.WriteString(" /* AFTER CONNECTION POOL ESTABLISHED */")
}There was a problem hiding this comment.
Yeah I would prefer Option 2 as well on this PR.
However, If this were a master-branch design cleanup, I’d lean to Option 1 (header matches executed SQL) plus a DM parser update for the after-connection binary-log header. Option 1 is more clear and aligned from a design perspective.
|
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:
📝 WalkthroughWalkthroughSwitches metadata recording to use full version.ServerInfo, adds a MySQL 8.4.0 terminology/version boundary, makes SHOW statements version-aware (SHOW MASTER STATUS / SHOW BINARY LOG STATUS / SHOW REPLICA STATUS), updates call signatures and tests, and adds MySQL 8.4.3 to CI. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Consistency Script
participant Dumpling as Dumpling Export
participant DB as Database
Script->>DB: QUERY VERSION()
DB-->>Script: version string
Script->>Dumpling: start export (ServerInfo(version))
Dumpling->>Dumpling: evaluate ServerInfo (type & version)
alt MySQL >= 8.4
Dumpling->>DB: SHOW BINARY LOG STATUS / SHOW REPLICA STATUS
else MariaDB or TiDB or older MySQL
Dumpling->>DB: SHOW MASTER STATUS (or fallback)
end
DB-->>Dumpling: metadata rows
Dumpling->>Dumpling: map columns (legacy ↔ new names)
Dumpling->>Dumpling: recordGlobalMetaData(serverInfo, mapped data)
Dumpling-->>Script: return metadata / snapshot
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
![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/tests/consistency/run.sh (1)
1-1: Consider using#!/bin/bashfor bash-specific syntax.The script uses bash-specific constructs (
[[ ]]and=~regex matching) on lines 35 and 38, but the shebang specifies POSIX/bin/sh. While this typically works in CI environments where/bin/shis symlinked to bash, it may fail on systems with a strict POSIX shell (e.g., dash on Debian/Ubuntu).🔧 Proposed fix
-#!/bin/sh +#!/bin/bash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dumpling/tests/consistency/run.sh` at line 1, The shebang is /bin/sh but the script uses bash-specific constructs ([[ ]] and =~ in the script), so update the script to be consistent: either change the shebang to #!/bin/bash to explicitly require bash, or replace the bash-specific conditionals (instances of [[ ... ]] and =~) with POSIX-compatible tests (e.g., use case, expr, or grep) so run.sh no longer relies on bash-specific features; locate the bash constructs in the file (the conditional checks using [[ and =~) and apply the chosen fix.
🤖 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`:
- Around line 206-211: The test creates a MySQL ServerInfo but is meant to
exercise MariaDB behavior; replace the call to version.ParseServerInfo("8.0.45")
with a MariaDB server string (e.g. version.ParseServerInfo("10.3.29-MariaDB"))
so that the ServerInfo used by recordGlobalMetaData (called on m via
newGlobalMetadata) has ServerType set to MariaDB and the code will query "SHOW
ALL SLAVES STATUS" as expected.
---
Nitpick comments:
In `@dumpling/tests/consistency/run.sh`:
- Line 1: The shebang is /bin/sh but the script uses bash-specific constructs
([[ ]] and =~ in the script), so update the script to be consistent: either
change the shebang to #!/bin/bash to explicitly require bash, or replace the
bash-specific conditionals (instances of [[ ... ]] and =~) with POSIX-compatible
tests (e.g., use case, expr, or grep) so run.sh no longer relies on
bash-specific features; locate the bash constructs in the file (the conditional
checks using [[ and =~) and apply the chosen fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7a6cab93-8bf5-4a15-8c5c-cb8496e03d68
📒 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
dveeden
force-pushed
the
dumpling_mysql84_tidb85
branch
from
195ab54 to
62b5f7d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dumpling/export/metadata_test.go (1)
43-45:⚠️ Potential issue | 🟠 MajorAdd an explicit MySQL 8.4 regression case for replica metadata parsing.
These MySQL test updates only exercise 8.0/5.5 paths; they still don’t cover the 8.4 branch (
SHOW REPLICA STATUS+Source_*/Exec_Source_*fields), which is central to this fix.#!/bin/bash set -euo pipefail file="$(fd '^metadata_test\.go$' | head -n1)" echo "Inspecting: ${file}" echo "== Existing 8.4 / replica-terminology coverage ==" rg -n '8\.4|SHOW REPLICA STATUS|exec_source_log_pos|relay_source_log_file|source_host' "${file}" || true echo echo "Expected: at least one dedicated MySQL 8.4 test should match these patterns."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: 77-80, 115-117, 149-151, 256-258
🤖 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 43 - 45, Add a dedicated MySQL 8.4 regression test in metadata_test.go that exercises the replica-terminology branch by constructing a ServerInfo for "8.4" via version.ParseServerInfo and calling m.recordGlobalMetaData(conn, si, false) (or the existing test helper) against a mock/result set that uses "SHOW REPLICA STATUS" column names (Source_*/Exec_Source_*/relay_source_log_file/exec_source_log_pos/source_host etc.); ensure the test fails against the old behavior and passes after the fix, mirroring the existing 8.0/5.5 tests but using the 8.4 ServerType/fields to validate replica metadata parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dumpling/export/metadata_test.go`:
- Around line 43-45: Add a dedicated MySQL 8.4 regression test in
metadata_test.go that exercises the replica-terminology branch by constructing a
ServerInfo for "8.4" via version.ParseServerInfo and calling
m.recordGlobalMetaData(conn, si, false) (or the existing test helper) against a
mock/result set that uses "SHOW REPLICA STATUS" column names
(Source_*/Exec_Source_*/relay_source_log_file/exec_source_log_pos/source_host
etc.); ensure the test fails against the old behavior and passes after the fix,
mirroring the existing 8.0/5.5 tests but using the 8.4 ServerType/fields to
validate replica metadata parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e2d9729e-48c5-41ed-a2f4-2bc4a46197cf
📒 Files selected for processing (2)
dumpling/export/metadata.godumpling/export/metadata_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dumpling/export/metadata_test.go (1)
57-81: Add an explicit8.4.0boundary test input.Current coverage checks
<8.4(8.0.45) and>8.4(8.4.8). Using8.4.0in one_84test would lock the threshold behavior and guard comparator regressions.♻️ Minimal boundary-hardening tweak
- si := version.ParseServerInfo("8.4.8") + si := version.ParseServerInfo("8.4.0")As per coding guidelines, "
**/*_test.go: MUST add a regression test and verify it fails before fix and passes after fix for bug fixes."Also applies to: 168-189
🤖 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 57 - 81, Add an explicit boundary test for version 8.4.0 by extending TestMysqlMetaData_84 (or adding a separate test) to call version.ParseServerInfo("8.4.0"), create the same test setup via newGlobalMetadata(...) and conn, assert the parsed ServerType equals version.ServerTypeMySQL and then call m.recordGlobalMetaData(conn, si, false) to verify behavior at the exact boundary; mirror the existing mock setup (SHOW BINARY LOG STATUS, SELECT @@default_master_connection error, SHOW REPLICA STATUS) so the test fails before any bug fix and passes after, thereby locking the comparator behavior.
🤖 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 182: The test fails because the SQL expectation string in the mock is
incorrect; update the ExpectQuery call in metadata_test.go (the mock setup for
ShowMasterStatus) to use the correct query "SHOW BINARY LOG STATUS" (single
space) so it matches the actual query emitted by ShowMasterStatus; locate the
ExpectQuery invocation that currently has "SHOW BINARY LOG STATUS" and correct
the spacing to match the real query.
---
Nitpick comments:
In `@dumpling/export/metadata_test.go`:
- Around line 57-81: Add an explicit boundary test for version 8.4.0 by
extending TestMysqlMetaData_84 (or adding a separate test) to call
version.ParseServerInfo("8.4.0"), create the same test setup via
newGlobalMetadata(...) and conn, assert the parsed ServerType equals
version.ServerTypeMySQL and then call m.recordGlobalMetaData(conn, si, false) to
verify behavior at the exact boundary; mirror the existing mock setup (SHOW
BINARY LOG STATUS, SELECT @@default_master_connection error, SHOW REPLICA
STATUS) so the test fails before any bug fix and passes after, thereby locking
the comparator behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bd89d322-ba52-4814-90ba-d274f0ce046f
📒 Files selected for processing (1)
dumpling/export/metadata_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 79-81: Add a regression assertion that uses the exact boundary
version "8.4.0": create a ServerInfo with version.ParseServerInfo("8.4.0"),
assert si.ServerType equals version.ServerTypeMySQL and call require.NoError(t,
m.recordGlobalMetaData(conn, si, false)). Place this alongside the existing
"8.4.8" test in metadata_test.go to catch >= vs > boundary regressions for
recordGlobalMetaData.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9975eb8d-34e5-4b00-a8f2-8bc4a4e5978c
📒 Files selected for processing (1)
dumpling/export/metadata_test.go
| si := version.ParseServerInfo("8.4.8") | ||
| require.Equal(t, version.ServerType(version.ServerTypeMySQL), si.ServerType) | ||
| require.NoError(t, m.recordGlobalMetaData(conn, si, false)) |
There was a problem hiding this comment.
Add an exact 8.4.0 boundary case in this regression path.
Line 79 currently uses 8.4.8, which validates 8.4+ behavior but not the threshold itself. Using 8.4.0 in at least one test catches > vs >= boundary regressions.
✅ Minimal tweak
- si := version.ParseServerInfo("8.4.8")
+ si := version.ParseServerInfo("8.4.0")As per coding guidelines **/*_test.go: "MUST add a regression test and verify it fails before fix and passes after fix for bug fixes."
🤖 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 79 - 81, Add a regression
assertion that uses the exact boundary version "8.4.0": create a ServerInfo with
version.ParseServerInfo("8.4.0"), assert si.ServerType equals
version.ServerTypeMySQL and call require.NoError(t, m.recordGlobalMetaData(conn,
si, false)). Place this alongside the existing "8.4.8" test in metadata_test.go
to catch >= vs > boundary regressions for recordGlobalMetaData.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dumpling/tests/s3/run.sh (1)
67-67: Reuse the exported MinIO settings here.Line 67 hardcodes the endpoint and credentials instead of reusing
S3_ENDPOINT,MINIO_ACCESS_KEY, andMINIO_SECRET_KEY. That makes the verification step drift-prone if the test setup changes later.♻️ Proposed fix
-bin/mc alias set minio http://127.0.0.1:5000 testid testkey8 +bin/mc alias set minio "http://$S3_ENDPOINT" "$MINIO_ACCESS_KEY" "$MINIO_SECRET_KEY"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dumpling/tests/s3/run.sh` at line 67, Replace the hardcoded MinIO endpoint and credentials in the alias setup command with the exported environment variables S3_ENDPOINT, MINIO_ACCESS_KEY, and MINIO_SECRET_KEY used elsewhere in the test; update the call that currently runs bin/mc alias set minio http://127.0.0.1:5000 testid testkey8 to reference S3_ENDPOINT, MINIO_ACCESS_KEY and MINIO_SECRET_KEY (properly quoted/expanded) so the verification step tracks changes to the test setup.
🤖 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/tests/s3/run.sh`:
- Line 67: Replace the hardcoded MinIO endpoint and credentials in the alias
setup command with the exported environment variables S3_ENDPOINT,
MINIO_ACCESS_KEY, and MINIO_SECRET_KEY used elsewhere in the test; update the
call that currently runs bin/mc alias set minio http://127.0.0.1:5000 testid
testkey8 to reference S3_ENDPOINT, MINIO_ACCESS_KEY and MINIO_SECRET_KEY
(properly quoted/expanded) so the verification step tracks changes to the test
setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b053446-3461-466b-a576-3839d029ed1e
📒 Files selected for processing (1)
dumpling/tests/s3/run.sh
|
/retest |
| @@ -53,6 +53,7 @@ jobs: | |||
| - 8.0.22 | |||
There was a problem hiding this comment.
maybe we can reduce some version of 8.0
There was a problem hiding this comment.
I'll do that in #66704 for the master branch.
| } | ||
|
|
||
| // MariaDB 10.11 replica without GTID | ||
| func TestMariaDBWithFollowersMetaData(t *testing.T) { |
There was a problem hiding this comment.
Is there another unit test to check MariaDB with GTID?
[LGTM Timeline notifier]Timeline:
|
|
/retest |
1 similar comment
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance6716, OliverS929 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 |
|
@OliverS929: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions 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. |
|
/retest |
4 participants
What problem does this PR solve?
Issue Number: ref #53082
Problem Summary:
What changed and how does it work?
This is a cherry-pick of #57188
This changes the syntax that Dumpling uses to be compatible with MySQL 8.4. For older versions it uses the old syntax.
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
New Features
Tests
Chores