ddl: `tidb_scatter_region` variable supports setting value in both upper/lower case #57677

River2000i · 2024-11-25T09:26:31Z

What problem does this PR solve?

Issue Number: close #57669

Problem Summary: variable should accept upper/lower case

What changed and how does it work?

convert value from uppercase to lowercase

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

…wer case

ti-chi-bot · 2024-11-25T09:26:42Z

Hi @River2000i. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

tiprow · 2024-11-25T09:26:48Z

Hi @River2000i. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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.

lance6716 · 2024-11-25T10:07:44Z

/ok-to-test

lance6716 · 2024-11-25T10:07:57Z

please check affected version of the issue

Benjamin2037 · 2024-11-25T10:24:33Z

pkg/ddl/table_split_test.go

@@ -111,6 +115,19 @@ func TestScatterRegion(t *testing.T) {
 	tk.MustQuery("select @@global.tidb_scatter_region;").Check(testkit.Rows("global"))
 	tk.MustExec("set global tidb_scatter_region = '';")
 	tk.MustQuery("select @@global.tidb_scatter_region;").Check(testkit.Rows(""))
+	tk2 = testkit.NewTestKit(t, store)


you can also add some mixed "TablE" cases.

add case in https://github.com/pingcap/tidb/pull/57677/files#diff-f9c16c705600da397c764153cc51c9835f6989854cd1614db6217eee18e8c318R141-R145

D3Hunter

rest lgtm except existing comments

ti-chi-bot · 2024-11-25T10:28:46Z

[LGTM Timeline notifier]

Timeline:

2024-11-25 10:07:46.659396765 +0000 UTC m=+458254.279051281: ☑️ agreed by lance6716.
2024-11-25 10:28:45.216810435 +0000 UTC m=+459512.836464950: ☑️ agreed by D3Hunter.

D3Hunter · 2024-11-25T10:28:53Z

/hold

you can unhold after fix the comment

codecov · 2024-11-25T10:33:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.0100%. Comparing base (702c4f2) to head (7c010d8).
Report is 13 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57677        +/-   ##
================================================
+ Coverage   72.7952%   75.0100%   +2.2147%     
================================================
  Files          1677       1722        +45     
  Lines        463842     475191     +11349     
================================================
+ Hits         337655     356441     +18786     
+ Misses       105314      96609      -8705     
- Partials      20873      22141      +1268

Flag	Coverage Δ
integration	`49.8593% <50.0000%> (?)`
unit	`72.5261% <100.0000%> (+0.3355%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.6827% <ø> (+16.1654%)`	⬆️

River2000i · 2024-11-25T13:12:31Z

/unhold

River2000i · 2024-11-26T03:25:10Z

@yudongusa @benmeadowcroft PTAL~

ti-chi-bot · 2024-11-26T18:42:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benmeadowcroft, D3Hunter, lance6716

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/ddl/OWNERS~~ [D3Hunter,lance6716]
~~pkg/sessionctx/variable/OWNERS~~ [benmeadowcroft]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-11-26T19:25:21Z

In response to a cherrypick label: new pull request created to branch release-8.5: #57741.

…per/lower case (#57677) (#57741) close #57669

River2000i added 2 commits November 25, 2024 17:23

tidb_scatter_region variable supports setting values in both upper/lo…

92c6486

…wer case

Merge branch 'master' of github.com:pingcap/tidb into fix-57669

a462473

River2000i changed the title ~~ddl: tidb_scatter_region variable supports setting value in both upper/lower case~~ ddl: tidb_scatter_region variable supports setting value in both upper/lower case Nov 25, 2024

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Nov 25, 2024

lance6716 approved these changes Nov 25, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Nov 25, 2024

fix

5c59bb4

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Nov 25, 2024

Benjamin2037 reviewed Nov 25, 2024

View reviewed changes

D3Hunter reviewed Nov 25, 2024

View reviewed changes

D3Hunter approved these changes Nov 25, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 25, 2024

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 25, 2024

add test

7c010d8

ti-chi-bot bot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 25, 2024

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. labels Nov 25, 2024

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 25, 2024

benmeadowcroft approved these changes Nov 26, 2024

View reviewed changes

ti-chi-bot bot added the approved label Nov 26, 2024

ti-chi-bot bot merged commit dcc9dcc into pingcap:master Nov 26, 2024
27 checks passed

ti-chi-bot mentioned this pull request Nov 26, 2024

ddl: tidb_scatter_region variable supports setting value in both upper/lower case (#57677) #57741

Merged

13 tasks

River2000i deleted the fix-57669 branch November 27, 2024 02:12

ti-chi-bot bot pushed a commit that referenced this pull request Nov 27, 2024

ddl: tidb_scatter_region variable supports setting value in both up…

6d1b76b

…per/lower case (#57677) (#57741) close #57669

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: `tidb_scatter_region` variable supports setting value in both upper/lower case #57677

ddl: `tidb_scatter_region` variable supports setting value in both upper/lower case #57677

River2000i commented Nov 25, 2024 •

edited

Loading

ti-chi-bot bot commented Nov 25, 2024

tiprow bot commented Nov 25, 2024

lance6716 commented Nov 25, 2024

lance6716 commented Nov 25, 2024

Benjamin2037 Nov 25, 2024 •

edited

Loading

River2000i Nov 25, 2024

D3Hunter left a comment

ti-chi-bot bot commented Nov 25, 2024

D3Hunter commented Nov 25, 2024

codecov bot commented Nov 25, 2024 •

edited

Loading

River2000i commented Nov 25, 2024

River2000i commented Nov 26, 2024 •

edited

Loading

ti-chi-bot bot commented Nov 26, 2024

ti-chi-bot commented Nov 26, 2024

ddl: tidb_scatter_region variable supports setting value in both upper/lower case #57677

ddl: tidb_scatter_region variable supports setting value in both upper/lower case #57677

Conversation

River2000i commented Nov 25, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Nov 25, 2024

tiprow bot commented Nov 25, 2024

lance6716 commented Nov 25, 2024

lance6716 commented Nov 25, 2024

Benjamin2037 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

River2000i Nov 25, 2024

Choose a reason for hiding this comment

D3Hunter left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 25, 2024

[LGTM Timeline notifier]

D3Hunter commented Nov 25, 2024

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

River2000i commented Nov 25, 2024

River2000i commented Nov 26, 2024 • edited Loading

ti-chi-bot bot commented Nov 26, 2024

ti-chi-bot commented Nov 26, 2024

ddl: `tidb_scatter_region` variable supports setting value in both upper/lower case #57677

ddl: `tidb_scatter_region` variable supports setting value in both upper/lower case #57677

River2000i commented Nov 25, 2024 •

edited

Loading

Benjamin2037 Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

River2000i commented Nov 26, 2024 •

edited

Loading