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 60.92715% with 59 lines in your changes missing coverage. Please review.

Project coverage is 74.7431%. Comparing base (fe1b9ed) to head (1f9a46c).
Report is 7 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57611        +/-   ##
================================================
+ Coverage   72.8112%   74.7431%   +1.9318%     
================================================
  Files          1676       1721        +45     
  Lines        463677     472929      +9252     
================================================
+ Hits         337609     353482     +15873     
+ Misses       105228      97195      -8033     
- Partials      20840      22252      +1412

Flag	Coverage Δ
integration	`49.4225% <25.1655%> (?)`
unit	`72.1905% <55.6291%> (-0.0215%)`	⬇️

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

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

---- 🚨 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 <1004626265@qq.com>

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

/cc @lance6716, please help take a look

…g_bwlimit

lance6716 · 2024-11-22T07:58:40Z

pkg/planner/core/planbuilder.go

@@ -5940,6 +5941,16 @@ func checkAlterDDLJobOptValue(opt *AlterDDLJobOpt) error {
 			return fmt.Errorf("the value %v for %s is out of range [%v, %v]",
 				bs, opt.Name, variable.MinDDLReorgBatchSize, variable.MaxDDLReorgBatchSize)
 		}
+	case AlterDDLJobMaxWriteSpeed:
+		speedStr := opt.Value.(*expression.Constant).Value.GetString()


I found there's a test case

admin alter ddl jobs 1 max_write_speed = 4

So the value may not be string type? GetString will not work

We only support the string type with quotation marks

tidb/pkg/parser/parser_test.go

Line 548 in bc03ed8

{"admin alter ddl jobs 1 max_write_speed = 4", true, "ADMIN ALTER DDL JOBS 1 max_write_speed = 4"},

Maybe you should change the parser syntax to only accept string

support int in 7d46d9e

lance6716 · 2024-11-22T08:01:48Z

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 {


in which case dm is nil? And we should handle nil dm in SetMaxWriteSpeed

I guess an old cluster without DDLReorgMeta field upgrade to a new cluster, after unmarshal, this dm can be nil

update in 7345329

lance6716 · 2024-11-22T08:03:55Z

pkg/lightning/backend/local/region_job.go

+}
+
+// GetLimiterSpeed returns the speed of the write limiter.
+func (local *Backend) GetLimiterSpeed() int {


write a unit test to check they are concurrent-safe

Do you mean like this 8c07af0 ?

lance6716 · 2024-11-22T08:09:00Z

pkg/ddl/backfilling.go

 	err := pipe.Execute()
 	if err != nil {
 		return err
 	}

-	// Adjust worker pool size dynamically.
+	// Adjust worker pool size and max write speed dynamically.
 	if job != nil {
 		go func() {


this is a problem in old code: We'd better wait the goroutine exited before leave this function, avoid the goroutine accesses something and causes data race with caller.

update in 50c4334, PTAL

lance6716 · 2024-11-22T08:10:14Z

pkg/executor/operate_ddl_jobs.go

+		case core.AlterDDLJobMaxWriteSpeed:
+			if opt.Value != nil {
+				cons := opt.Value.(*expression.Constant)
+				speed, err := units.RAMInBytes(cons.Value.GetString())


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 <1004626265@qq.com>

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

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

eb01652

…g_bwlimit

lance6716 reviewed Nov 22, 2024

View reviewed changes

fzzf678 added 3 commits November 22, 2024 17:13

wait goroutine exit

50c4334

support int type receive value

7d46d9e

handle the nil case in Set

7345329

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

fzzf678 added 4 commits November 22, 2024 17:54

Update planbuilder.go

236a688

Update planbuilder.go

15e2726

add test

8c07af0

fix test

1f9a46c

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

lance6716 Nov 22, 2024

fzzf678 Nov 22, 2024

lance6716 Nov 22, 2024

fzzf678 Nov 22, 2024

lance6716 Nov 22, 2024

fzzf678 Nov 22, 2024

fzzf678 Nov 22, 2024 •

edited

Loading

lance6716 Nov 22, 2024

fzzf678 Nov 22, 2024

lance6716 Nov 22, 2024

fzzf678 Nov 22, 2024

lance6716 Nov 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fzzf678 Nov 22, 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

fzzf678 commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

fzzf678 Nov 22, 2024 •

edited

Loading