ddl: dynamically adjusting the max write speed of reorganization job #57611

fzzf678 · 2024-11-21T16:12:20Z

What problem does this PR solve?

Issue Number: ref #57526 #57229

Problem Summary: See #57229

What changed and how does it work?

Supply later

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

tiprow · 2024-11-21T16:12:45Z

Hi @fzzf678. 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.

fzzf678 · 2024-11-21T16:19:03Z

/ok-to-test

fzzf678 · 2024-11-21T16:23:14Z

/test all

codecov · 2024-11-21T16:44:43Z

Codecov Report

Attention: Patch coverage is 66.32653% with 33 lines in your changes missing coverage. Please review.

Project coverage is 74.1107%. Comparing base (6e22b8c) to head (3173317).
Report is 6 commits behind head on master.

❗ Current head 3173317 differs from pull request most recent head a1cc392

Please upload reports for the commit a1cc392 to get more accurate results.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57611        +/-   ##
================================================
+ Coverage   72.7907%   74.1107%   +1.3199%     
================================================
  Files          1676       1721        +45     
  Lines        463750     471832      +8082     
================================================
+ Hits         337567     349678     +12111     
+ Misses       105323      99849      -5474     
- Partials      20860      22305      +1445

Flag	Coverage Δ
integration	`46.8841% <48.8888%> (?)`

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

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

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

CbcWestwolf · 2024-11-22T06:29:32Z

pkg/meta/model/reorg.go

@@ -75,8 +75,9 @@ type DDLReorgMeta struct {
 	// These two variables are used to control the concurrency and batch size of the reorganization process.
 	// They can be adjusted dynamically through `admin alter ddl jobs` command.
 	// Note: Don't get or set these two variables directly, use the functions instead.


Please make these 3 var private

OK, I will try make these vars private in next PR

CbcWestwolf

Rest LGTM

pkg/meta/model/reorg.go

ti-chi-bot · 2024-11-22T06:33:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: CbcWestwolf, winoros
Once this PR has been reviewed and has the lgtm label, please assign gmhdbjd for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

~~OWNERS~~ [CbcWestwolf,winoros]
pkg/ddl/OWNERS
pkg/lightning/OWNERS
pkg/meta/OWNERS
~~pkg/planner/OWNERS~~ [winoros]

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-22T06:33:41Z

[LGTM Timeline notifier]

Timeline:

2024-11-22 06:03:08.187779729 +0000 UTC m=+184375.807434246: ☑️ agreed by winoros.
2024-11-22 06:33:40.782162927 +0000 UTC m=+186208.401817441: ☑️ agreed by CbcWestwolf.

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

fzzf678 · 2024-11-22T07:37:33Z

/cc @lance6716, please help take a look

pkg/ddl/backfilling.go

pkg/lightning/backend/local/localhelper.go

pkg/lightning/backend/local/region_job.go

D3Hunter · 2024-11-25T02:41:15Z

pkg/meta/model/reorg.go

+// GetMaxWriteSpeedOrDefault gets the max write speed from DDLReorgMeta.
+// 0 means no limit.
+func (dm *DDLReorgMeta) GetMaxWriteSpeedOrDefault(defaultVal int) int {
+	if dm == nil {


this should be handled in where it's unmarshalled in a unified way, not check in every method

D3Hunter · 2024-11-25T02:41:31Z

pkg/meta/model/reorg.go

+	if dm == nil {
+		return defaultVal
+	}
+	return int(atomic.LoadInt64(&dm.MaxWriteSpeed))


use atomic.XXX type

I guess the reason is atomic.XXX does not implement (un)marshal interface, so we must store the plain type in struct and use helper functions

golang's doesn't, but uber's does

~~If we use the atomic.XXX type, seems these three fields wiil meet unmarshal error when the job meta is from old version(upgrade)?~~ Seems not, store int64 in the under layer

// MarshalJSON encodes the wrapped int64 into JSON. func (i *Int64) MarshalJSON() ([]byte, error) { return json.Marshal(i.Load()) } // UnmarshalJSON decodes JSON into the wrapped int64. func (i *Int64) UnmarshalJSON(b []byte) error { var v int64 if err := json.Unmarshal(b, &v); err != nil { return err } i.Store(v) return nil }

pkg/executor/operate_ddl_jobs.go

pkg/lightning/backend/local/localhelper_test.go

pkg/lightning/backend/local/localhelper.go

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

use util.WaitGroupWrapper rename limit related func use atomic.int64 in storeWriteLimiter remove noopStoreWriteLimit

…g_bwlimit

D3Hunter · 2024-11-25T05:30:49Z

pkg/meta/model/job.go

@@ -546,6 +546,9 @@ func (job *Job) Encode(updateRawArgs bool) ([]byte, error) {
 // decode special args for this job.
 func (job *Job) Decode(b []byte) error {
 	err := json.Unmarshal(b, job)
+	if job.MayNeedReorg() && job.ReorgMeta == nil {


MayNeedReorg depends on a context var set at DDL executor, when job worker run it, it's not there

maybe move it to where job.ReorgMeta is used

update in a128426 PTAL again

D3Hunter · 2024-11-25T05:45:02Z

rest lgtm

pkg/planner/core/planbuilder_test.go

pkg/planner/core/planbuilder.go

lance6716 · 2024-11-25T06:33:05Z

pkg/planner/core/planbuilder.go

+		speed int64
+		err   error
+	)
+	v := opt.Value.(*expression.Constant)


add unit tests to make sure it will always be a constant. For example, ALTER ... max_write_speed = RAND() should fail with friendly error message

lance6716 · 2024-11-25T06:34:38Z

pkg/planner/core/planbuilder.go

+	case types.ETInt:
+		speed = v.Value.GetInt64()
+	default:
+		return 0, fmt.Errorf("the value %s for %s is invalid", opt.Name, opt.Value)


add a unit test to check the error message is readable in this case.

lance6716 · 2024-11-25T06:34:51Z

pkg/planner/core/planbuilder.go

+		speedStr := v.Value.GetString()
+		speed, err = units.RAMInBytes(speedStr)
+		if err != nil {
+			return 0, errors.Trace(err)


also check error mesage is readable when failed to parse

lance6716 · 2024-11-25T06:43:45Z

pkg/lightning/backend/local/localhelper_test.go

+					gotTokens += n
+				}
+				elapsed := time.Since(start)
+				maxTokens := 120 + int(float64(elapsed.Seconds())*float64(maxT))


elapsed.Seconds() is already float64. Your IDE should notice this for you 🤔

lance6716 · 2024-11-25T06:47:01Z

pkg/lightning/backend/local/localhelper_test.go

+				elapsed := time.Since(start)
+				maxTokens := 120 + int(float64(elapsed.Seconds())*float64(maxT))
+				// In theory, gotTokens should be less than or equal to maxT.
+				// But we allow a little of error to avoid the test being flaky.


Did you find this test is flaky? I think the sleep-related functions guarantee that at least some duration it will block. So the gotTokens should always be less or equal than maxT * elapsed.

Oh I see it's because out limiter "Allow burst of at most 20% of the writeLimit." We can change hardcoded 120 to 1.2*maxT then.

update in a1cc392

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

fzzf678 · 2024-11-25T07:12:21Z

pkg/ddl/db_test.go

@@ -1195,6 +1195,17 @@ func TestAdminAlterDDLJobUnsupportedCases(t *testing.T) {
 	tk.MustGetErrMsg("admin alter ddl jobs 1 thread = 257;", "the value 257 for thread is out of range [1, 256]")
 	tk.MustGetErrMsg("admin alter ddl jobs 1 batch_size = 31;", "the value 31 for batch_size is out of range [32, 10240]")
 	tk.MustGetErrMsg("admin alter ddl jobs 1 batch_size = 10241;", "the value 10241 for batch_size is out of range [32, 10240]")
+	tk.MustGetErrMsg("admin alter ddl jobs 1 max_write_speed = '2PiB';", "the value 2PiB for max_write_speed is out of range [0, 1125899906842624]")


some error message cc @lance6716

The negative tests cases are not enough. Please check at least max_write_speed = 1.23, max_write_speed = RAND(), max_write_speed = 30+40, max_write_speed = 'asdasd', and maybe ask other reviewers to come up with more cases.

add some cases and fix comment in d71a826 PTAL again

tiprow · 2024-11-25T09:06:09Z

@fzzf678: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	`a1cc392`	link	true	`/test fast_test_tiprow`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

fzzf678 added 5 commits November 19, 2024 19:47

build execute max_write_speed

745c0bb

Merge remote-tracking branch 'upstream/master' into polling_bwlimit

2bdcee7

use get and set interface handle the max write speed in reorg meta

f286233

make the limiter can be adjusted

cbbd6ab

adjust max write speed

829c67f

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 21, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Nov 21, 2024

Update BUILD.bazel

07869e3

fzzf678 added 3 commits November 22, 2024 09:29

fix nogo

b7eed57

fix absurd

3e3955f

show ddl jobs print max write speed

fb8422b

winoros approved these changes Nov 22, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 22, 2024

rename

cc98597

CbcWestwolf reviewed Nov 22, 2024

View reviewed changes

CbcWestwolf approved these changes Nov 22, 2024

View reviewed changes

pkg/meta/model/reorg.go Outdated Show resolved Hide resolved

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

Update pkg/meta/model/reorg.go

bb381f9

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

fzzf678 requested a review from lance6716 November 22, 2024 07:40

Merge remote-tracking branch 'upstream/master' into polling_bwlimit

5fbf892

D3Hunter reviewed Nov 25, 2024

View reviewed changes

pkg/ddl/backfilling.go Outdated Show resolved Hide resolved

pkg/lightning/backend/local/localhelper.go Outdated Show resolved Hide resolved

pkg/lightning/backend/local/localhelper.go Outdated Show resolved Hide resolved

D3Hunter reviewed Nov 25, 2024

View reviewed changes

pkg/executor/operate_ddl_jobs.go Outdated Show resolved Hide resolved

pkg/lightning/backend/local/localhelper_test.go Outdated Show resolved Hide resolved

pkg/lightning/backend/local/localhelper.go Outdated Show resolved Hide resolved

fzzf678 and others added 7 commits November 25, 2024 11:14

Update pkg/lightning/backend/local/localhelper_test.go

c1ea4f0

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

fix some coment

fc28a05

use util.WaitGroupWrapper rename limit related func use atomic.int64 in storeWriteLimiter remove noopStoreWriteLimit

merge duplicate code

62cb8f3

handle the reorg meta nil case when unmarshalling

1b25f9a

Merge branch 'polling_bwlimit' of github.com:fzzf678/tidb into pollin…

845615c

…g_bwlimit

use uber's atomic

3173317

update bazel

1158823

D3Hunter reviewed Nov 25, 2024

View reviewed changes

fzzf678 added 2 commits November 25, 2024 14:44

handle the reorgmeta nil

a128426

remove useless param

2a29bd5

lance6716 reviewed Nov 25, 2024

View reviewed changes

fzzf678 and others added 5 commits November 25, 2024 14:48

Update pkg/planner/core/planbuilder_test.go

0252822

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

Update pkg/planner/core/planbuilder_test.go

4237d3c

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

Update pkg/planner/core/planbuilder.go

3900fe9

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

use require.Len()

941af08

use named return value

f7cce41

fzzf678 commented Nov 25, 2024

View reviewed changes

fzzf678 added 5 commits November 25, 2024 16:23

add test and fix comment

d71a826

Update planbuilder_test.go

5ba4278

check thread and batch_size eval type

a9cda92

Update BUILD.bazel

00d0027

correct maxTokens formula

a1cc392

pingcap deleted a comment from ti-chi-bot bot Nov 25, 2024

pingcap deleted a comment from tiprow bot Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: dynamically adjusting the max write speed of reorganization job #57611

ddl: dynamically adjusting the max write speed of reorganization job #57611

fzzf678 commented Nov 21, 2024 •

edited

Loading

tiprow bot commented Nov 21, 2024

fzzf678 commented Nov 21, 2024

fzzf678 commented Nov 21, 2024

codecov bot commented Nov 21, 2024 •

edited

Loading

CbcWestwolf Nov 22, 2024

fzzf678 Nov 22, 2024

CbcWestwolf left a comment

ti-chi-bot bot commented Nov 22, 2024

ti-chi-bot bot commented Nov 22, 2024

fzzf678 commented Nov 22, 2024

D3Hunter Nov 25, 2024

D3Hunter Nov 25, 2024

lance6716 Nov 25, 2024

D3Hunter Nov 25, 2024

fzzf678 Nov 25, 2024 •

edited

Loading

D3Hunter Nov 25, 2024

fzzf678 Nov 25, 2024

D3Hunter commented Nov 25, 2024

lance6716 Nov 25, 2024 •

edited

Loading

lance6716 Nov 25, 2024

lance6716 Nov 25, 2024

lance6716 Nov 25, 2024

lance6716 Nov 25, 2024

lance6716 Nov 25, 2024

fzzf678 Nov 25, 2024

fzzf678 Nov 25, 2024

lance6716 Nov 25, 2024 •

edited

Loading

fzzf678 Nov 25, 2024 •

edited

Loading

tiprow bot commented Nov 25, 2024

ddl: dynamically adjusting the max write speed of reorganization job #57611

Are you sure you want to change the base?

ddl: dynamically adjusting the max write speed of reorganization job #57611

Conversation

fzzf678 commented Nov 21, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Nov 21, 2024

fzzf678 commented Nov 21, 2024

fzzf678 commented Nov 21, 2024

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CbcWestwolf left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 22, 2024

ti-chi-bot bot commented Nov 22, 2024

[LGTM Timeline notifier]

fzzf678 commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fzzf678 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D3Hunter commented Nov 25, 2024

lance6716 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

fzzf678 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

tiprow bot commented Nov 25, 2024

fzzf678 commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

fzzf678 Nov 25, 2024 •

edited

Loading

lance6716 Nov 25, 2024 •

edited

Loading

lance6716 Nov 25, 2024 •

edited

Loading

fzzf678 Nov 25, 2024 •

edited

Loading