DM: Use lower case for Sysvar 'gtid_mode' for better upstream compatibility. (#12169) #12257
Conversation
Signed-off-by: ti-chi-bot <[email protected]>
ti-chi-bot
added
area/dm
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
ti-chi-bot
bot
added
do-not-merge/cherry-pick-not-approved
size/L
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ti-chi-bot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily focuses on enhancing compatibility with upstream MySQL variants by standardizing the query for the gtid_mode system variable to its lowercase form. It also involves updates to the GetGTIDMode function's signature and corresponding test cases, with additional test functions being introduced as part of the automated cherry-pick.
Highlights
- GTID Mode Query Standardization: The
GetGTIDModefunction has been updated to query thegtid_modesystem variable using its lowercase name ("gtid_mode") instead of the uppercase ("GTID_MODE"). This change aims to improve compatibility with various upstream MySQL distributions, such as txsql. - Function Signature Update: The signature of the
GetGTIDModefunction indm/pkg/utils/db.gohas been modified. It now accepts*tcontext.Contextand*BaseDBas parameters, replacing the more genericcontext.Contextand*sql.DBtypes. This indicates a move towards more specific internal type usage for context and database connections. - Test Suite Refactoring and Expansion: The unit tests in
dm/pkg/utils/db_test.gohave been updated to align with the newGetGTIDModefunction signature and the lowercasegtid_modequery. Additionally, several new test functions (TestFetchAllDoTables,TestFetchTargetDoTables, etc.) have been introduced, which appear to be part of a broader set of changes integrated via the automated cherry-pick process.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
/cherry-pick-invite OliverS929 |
|
@OliverS929 Please accept the invitation then you can push to the cherry-pick pull requests. |
There was a problem hiding this comment.
Code Review
This pull request aims to change the system variable 'GTID_MODE' to lowercase for better upstream compatibility. However, the changes introduce several merge conflicts across multiple files, which will prevent the code from compiling. These conflicts must be resolved before the PR can be merged. It appears an automated cherry-pick has caused these issues due to significant divergence between branches, including file refactoring and moves.
dm/pkg/utils/db.go
Outdated
| <<<<<<< HEAD:dm/pkg/utils/db.go | ||
| // GetGTIDMode return GTID_MODE. | ||
| func GetGTIDMode(ctx context.Context, db *sql.DB) (string, error) { | ||
| val, err := GetGlobalVariable(ctx, db, "GTID_MODE") | ||
| ======= | ||
| // GetGTIDMode return gtid_mode (lowercase for better upstream compatibility, e.g., txsql). | ||
| func GetGTIDMode(ctx *tcontext.Context, db *BaseDB) (string, error) { | ||
| val, err := GetGlobalVariable(ctx, db, "gtid_mode") | ||
| >>>>>>> 8f20364e59 (DM: Use lower case for Sysvar 'gtid_mode' for better upstream compatibility. (#12169)):dm/pkg/conn/db.go |
There was a problem hiding this comment.
This file contains unresolved merge conflict markers (<<<<<<<, =======, >>>>>>>), which will prevent the code from compiling.
It appears this conflict arose from an automated cherry-pick onto a branch where GetGTIDMode was refactored and possibly moved. The function signature in the conflicting change uses types (*tcontext.Context, *BaseDB) that are not available in this file's scope.
Please resolve this conflict by removing the markers and ensuring there is a single, correct implementation of the function. You may need to adjust the PR to apply the change in the correct file.
dm/pkg/utils/db_test.go
Outdated
| <<<<<<< HEAD:dm/pkg/utils/db_test.go | ||
| rows := mock.NewRows([]string{"Variable_name", "Value"}).AddRow("GTID_MODE", "ON") | ||
| mock.ExpectQuery(`SHOW GLOBAL VARIABLES LIKE 'GTID_MODE'`).WillReturnRows(rows) | ||
| mode, err := GetGTIDMode(ctx, db) | ||
| ======= | ||
| rows := mock.NewRows([]string{"Variable_name", "Value"}).AddRow("gtid_mode", "ON") | ||
| mock.ExpectQuery(`SHOW GLOBAL VARIABLES LIKE 'gtid_mode'`).WillReturnRows(rows) | ||
| mode, err := GetGTIDMode(tctx, NewBaseDBForTest(db)) | ||
| >>>>>>> 8f20364e59 (DM: Use lower case for Sysvar 'gtid_mode' for better upstream compatibility. (#12169)):dm/pkg/conn/db_test.go |
There was a problem hiding this comment.
This test contains unresolved merge conflict markers, which will cause compilation to fail. This seems related to the conflict in dm/pkg/utils/db.go.
Please resolve this conflict. The call to GetGTIDMode here is also inconsistent, reflecting the refactoring that likely caused the conflict.
dm/pkg/utils/db_test.go
Outdated
| require.Len(t, got, 1) | ||
| require.Equal(t, []string{"tbl1", "tbl2"}, got[doSchema]) | ||
| require.NoError(t, mock.ExpectationsWereMet()) | ||
| } | ||
|
|
||
| func TestFetchTargetDoTables(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| db, mock, err := sqlmock.New() | ||
| require.NoError(t, err) | ||
|
|
||
| // empty filter and router, just as upstream. | ||
| ba, err := filter.New(false, nil) | ||
| require.NoError(t, err) | ||
| r, err := regexprrouter.NewRegExprRouter(false, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| schemas := []string{"shard1"} | ||
| rows := sqlmock.NewRows([]string{"Database"}) | ||
| addRowsForSchemas(rows, schemas) | ||
| mock.ExpectQuery(`SHOW DATABASES`).WillReturnRows(rows) | ||
|
|
||
| tablesM := map[string][]string{ | ||
| "shard1": {"tbl1", "tbl2"}, | ||
| } | ||
| for schema, tables := range tablesM { | ||
| rows = sqlmock.NewRows([]string{fmt.Sprintf("Tables_in_%s", schema), "Table_type"}) | ||
| addRowsForTables(rows, tables) | ||
| mock.ExpectQuery(fmt.Sprintf("SHOW FULL TABLES IN `%s` WHERE Table_Type != 'VIEW'", schema)).WillReturnRows(rows) | ||
| } | ||
|
|
||
| tablesMap, extendedCols, err := FetchTargetDoTables(context.Background(), "", NewBaseDBForTest(db), ba, r) | ||
| require.NoError(t, err) | ||
| require.Equal(t, map[filter.Table][]filter.Table{ | ||
| {Schema: "shard1", Name: "tbl1"}: {{Schema: "shard1", Name: "tbl1"}}, | ||
| {Schema: "shard1", Name: "tbl2"}: {{Schema: "shard1", Name: "tbl2"}}, | ||
| }, tablesMap) | ||
| require.Len(t, extendedCols, 0) | ||
| require.NoError(t, mock.ExpectationsWereMet()) | ||
|
|
||
| // route to the same downstream. | ||
| r, err = regexprrouter.NewRegExprRouter(false, []*router.TableRule{ | ||
| {SchemaPattern: "shard*", TablePattern: "tbl*", TargetSchema: "shard", TargetTable: "tbl"}, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| rows = sqlmock.NewRows([]string{"Database"}) | ||
| addRowsForSchemas(rows, schemas) | ||
| mock.ExpectQuery(`SHOW DATABASES`).WillReturnRows(rows) | ||
| for schema, tables := range tablesM { | ||
| rows = sqlmock.NewRows([]string{fmt.Sprintf("Tables_in_%s", schema), "Table_type"}) | ||
| addRowsForTables(rows, tables) | ||
| mock.ExpectQuery(fmt.Sprintf("SHOW FULL TABLES IN `%s` WHERE Table_Type != 'VIEW'", schema)).WillReturnRows(rows) | ||
| } | ||
|
|
||
| tablesMap, extendedCols, err = FetchTargetDoTables(context.Background(), "", NewBaseDBForTest(db), ba, r) | ||
| require.NoError(t, err) | ||
| require.Equal(t, map[filter.Table][]filter.Table{ | ||
| {Schema: "shard", Name: "tbl"}: { | ||
| {Schema: "shard1", Name: "tbl1"}, | ||
| {Schema: "shard1", Name: "tbl2"}, | ||
| }, | ||
| }, tablesMap) | ||
| require.Len(t, extendedCols, 0) | ||
| require.NoError(t, mock.ExpectationsWereMet()) | ||
| } | ||
|
|
||
| func addRowsForSchemas(rows *sqlmock.Rows, schemas []string) { | ||
| for _, d := range schemas { | ||
| rows.AddRow(d) | ||
| } | ||
| } | ||
|
|
||
| func addRowsForTables(rows *sqlmock.Rows, tables []string) { | ||
| for _, table := range tables { | ||
| rows.AddRow(table, "BASE TABLE") | ||
| } | ||
| } | ||
|
|
||
| func TestGetGTIDMode(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| db, mock, err := sqlmock.New() | ||
| require.NoError(t, err) | ||
|
|
||
| tctx := tcontext.NewContext(context.Background(), log.L()) | ||
|
|
||
| // Mock the query for GetGlobalVariable | ||
| rows := mock.NewRows([]string{"Variable_name", "Value"}).AddRow("gtid_mode", "ON") | ||
| mock.ExpectQuery(`SHOW GLOBAL VARIABLES LIKE 'gtid_mode'`).WillReturnRows(rows) | ||
|
|
||
| // Call the actual GetGTIDMode function | ||
| gtidMode, err := GetGTIDMode(tctx, NewBaseDBForTest(db)) | ||
|
|
||
| // Assert the results when we use lowercase gtid_mode | ||
| require.NoError(t, err) | ||
| require.Equal(t, "ON", gtidMode) | ||
| require.NoError(t, mock.ExpectationsWereMet()) | ||
| } | ||
| >>>>>>> 8f20364e59 (DM: Use lower case for Sysvar 'gtid_mode' for better upstream compatibility. (#12169)):dm/pkg/conn/db_test.go |
There was a problem hiding this comment.
This part of the file also contains merge conflict markers. It seems to be attempting to add a large block of new tests, but the <<<<<<< and >>>>>>> markers make the code invalid.
The path in the marker (dm/pkg/conn/db_test.go) suggests these tests might belong to a different file. Please resolve this conflict and ensure the new tests are placed in the correct location.
ti-chi-bot
bot
added
size/M
|
/retest |
1 similar comment
|
/retest |
This is an automated cherry-pick of #12169
What problem does this PR solve?
Issue Number: close #12167
What is changed and how it works?
Use lower case for Sysvar 'gtid_mode' for better upstream compatibility (i.e. txsql).
Check 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