ddl: get scatter variable from executor session context #61331

River2000i · 2025-05-26T14:04:04Z

What problem does this PR solve?

Issue Number: close #61321

Problem Summary: truncate table\truncate partition\add partition use incorrect session context

What changed and how does it work?

get tidb_scatter_region from executor session context

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

ti-chi-bot · 2025-05-26T14:04:14Z

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 · 2025-05-26T14:04:21Z

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.

Benjamin2037 · 2025-05-26T22:32:31Z

Please make sure to add testcases to check that the system variable can be set and gets the correct value。

Benjamin2037 · 2025-05-27T08:53:41Z

pkg/ddl/executor.go

+func getScatterScopeFromSessionctx(sctx sessionctx.Context) string {
+	var scatterScope string
+	val, ok := sctx.GetSessionVars().GetSystemVar(vardef.TiDBScatterRegion)
+	if !ok {


What situation will cause !ok？

If the variable didn't set by user, it will return !ok

Then I think there should not output Warn("get system variable met problem, won't scatter region")， Just one info, the system var did not be set, use the default value.

fix in dc069d3

shouldn't we use global setting if not set in session level?

tests/realtikvtest/ddltest/main_test.go

tangenta · 2025-05-27T11:38:47Z

tests/realtikvtest/ddltest/main_test.go

+// FullMode is a flag identify it should be run in full mode.
+// In full mode, the test will run all the cases.
+var FullMode = flag.Bool("full-mode", false, "whether tests run in full mode")
+


Suggested change

// FullMode is a flag identify it should be run in full mode.

// In full mode, the test will run all the cases.

var FullMode = flag.Bool("full-mode", false, "whether tests run in full mode")

No usage?

fixed, remove it

Benjamin2037 · 2025-05-27T11:42:21Z

/ok-to-test

Benjamin2037

rest LGTM

codecov · 2025-05-27T11:59:06Z

Codecov Report

Attention: Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.8800%. Comparing base (3634a3c) to head (d905f8b).
Report is 21 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #61331        +/-   ##
================================================
+ Coverage   73.1772%   74.8800%   +1.7027%     
================================================
  Files          1726       1748        +22     
  Lines        478575     491660     +13085     
================================================
+ Hits         350208     368155     +17947     
+ Misses       106896     100497      -6399     
- Partials      21471      23008      +1537

Flag	Coverage Δ
integration	`48.7887% <86.7924%> (?)`
unit	`72.4694% <98.1132%> (+0.0231%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7804% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.3035% <ø> (+13.7304%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Co-authored-by: tangenta <[email protected]>

River2000i · 2025-05-28T13:40:18Z

/retest

Benjamin2037 · 2025-05-28T14:10:07Z

shouldn't we use global setting if not set in session level?

I think it's not reasonable that user didn't set session level variable but we use global level. Since we need to keep the same session level variable with user session

so what's the value if user session haven't set it, the value of the global one?

it will use "", which means not scatter region

What if user set global variable，but not set session level，then get session level variable will get global level value， right？

River2000i · 2025-05-28T14:23:06Z

/retest

D3Hunter

rest lgtm

D3Hunter · 2025-05-28T14:44:08Z

pkg/ddl/executor.go

+		var scatterScope string
+		if val, ok := jobW.GetSessionVars(vardef.TiDBScatterRegion); ok {
+			scatterScope = val
+		}


only need get once

ti-chi-bot · 2025-05-28T14:46:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, D3Hunter

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:

~~OWNERS~~ [Benjamin2037,D3Hunter]
~~pkg/ddl/OWNERS~~ [Benjamin2037,D3Hunter]
~~pkg/meta/OWNERS~~ [Benjamin2037,D3Hunter]

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 · 2025-05-28T14:46:08Z

[LGTM Timeline notifier]

Timeline:

2025-05-27 11:45:25.038452015 +0000 UTC m=+355855.410239461: ☑️ agreed by Benjamin2037.
2025-05-28 14:46:07.555526241 +0000 UTC m=+30188.617543862: ☑️ agreed by D3Hunter.

River2000i · 2025-05-28T14:48:25Z

/unhold

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2025-05-28T15:37:36Z

In response to a cherrypick label: new pull request created to branch release-8.5: #61388.
But this PR has conflicts, please resolve them!

close pingcap#61321

) close #61321

…ingcap#61331)"

…context (pingcap#61331)"" because it has multiple dependent commits. This reverts commit 885f1ef.

get scatter variable from executor session context

c094fb9

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label May 26, 2025

River2000i added 2 commits May 27, 2025 11:00

update comment

0df6a07

add TestTiDBScatterRegion test

ccc4a2f

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 27, 2025

River2000i changed the title ~~[WIP]ddl: get scatter variable from executor session context~~ ddl: get scatter variable from executor session context May 27, 2025

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2025

Benjamin2037 reviewed May 27, 2025

View reviewed changes

tangenta reviewed May 27, 2025

View reviewed changes

tests/realtikvtest/ddltest/main_test.go Outdated Show resolved Hide resolved

tangenta reviewed May 27, 2025

View reviewed changes

ti-chi-bot bot added 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. do-not-merge/needs-triage-completed labels May 27, 2025

Benjamin2037 approved these changes May 27, 2025

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 27, 2025

River2000i and others added 2 commits May 27, 2025 20:36

update copyright

048a455

Co-authored-by: tangenta <[email protected]>

remove unused code

0d3dd29

ti-chi-bot bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 28, 2025

River2000i added 3 commits May 28, 2025 20:14

revert

4eaa53a

fix TestJobSize

896a625

revert

4955924

Benjamin2037 added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label May 28, 2025

remove AddSessionVars conflict

78d8f16

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 28, 2025

Benjamin2037 requested a review from tangenta May 28, 2025 14:29

D3Hunter approved these changes May 28, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 28, 2025

get once scatterScope

d905f8b

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 May 28, 2025

ti-chi-bot bot merged commit eeccd2e into pingcap:master May 28, 2025
27 checks passed

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request May 28, 2025

This is an automated cherry-pick of pingcap#61331

c4b11ec

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request May 28, 2025

ddl: get scatter variable from executor session context (#61331) #61388

Merged

13 tasks

River2000i deleted the fix61321 branch May 29, 2025 01:10

hawkingrei pushed a commit to hawkingrei/tidb that referenced this pull request May 29, 2025

ddl: get scatter variable from executor session context (pingcap#61331)

a90b101

close pingcap#61321

ti-chi-bot bot pushed a commit that referenced this pull request May 29, 2025

ddl: get scatter variable from executor session context (#61331) (#61388

f43a133

) close #61321

OliverS929 pushed a commit to OliverS929/tidb that referenced this pull request Jul 2, 2025

cherry-pick "ddl: get scatter variable from executor session context (p…

885f1ef

…ingcap#61331)"

OliverS929 mentioned this pull request Jul 2, 2025

*: Import Into support full-text index using TiCI #61760

Merged

13 tasks

OliverS929 added a commit to OliverS929/tidb that referenced this pull request Jul 2, 2025

Revert "cherry-pick "ddl: get scatter variable from executor session …

3f8b2a3

…context (pingcap#61331)"" because it has multiple dependent commits. This reverts commit 885f1ef.

	// FullMode is a flag identify it should be run in full mode.
	// In full mode, the test will run all the cases.
	var FullMode = flag.Bool("full-mode", false, "whether tests run in full mode")

ddl: get scatter variable from executor session context #61331

ddl: get scatter variable from executor session context #61331

Uh oh!

Conversation

River2000i commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented May 26, 2025

Uh oh!

tiprow bot commented May 26, 2025

Uh oh!

Benjamin2037 commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

River2000i May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Benjamin2037 commented May 27, 2025

Uh oh!

Benjamin2037 left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

River2000i commented May 28, 2025

Uh oh!

Benjamin2037 commented May 28, 2025

Uh oh!

River2000i commented May 28, 2025

Uh oh!

D3Hunter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented May 28, 2025

Uh oh!

ti-chi-bot bot commented May 28, 2025

[LGTM Timeline notifier]

Uh oh!

River2000i commented May 28, 2025

Uh oh!

Uh oh!

ti-chi-bot commented May 28, 2025

Uh oh!

Uh oh!

River2000i commented May 26, 2025 •

edited

Loading

Benjamin2037 commented May 26, 2025 •

edited

Loading

River2000i May 27, 2025 •

edited

Loading

codecov bot commented May 27, 2025 •

edited

Loading