ddl: part 1 of add-index integrating dxf dynamic modify param #59046

D3Hunter · 2025-01-20T13:55:10Z

What problem does this PR solve?

Issue Number: ref #57497, ref #57229

Problem Summary:

What changed and how does it work?

This is the first part, will add integrate test later

add a routine in executeDistTask to detect whether there are param mismatch between DDL job and DXF task, and we call ModifyTaskByID to let DXF handle the rest param modify work if detected
- there is a way to achieve this without start a routine, but requires refactor transitOneJobStep to start txn on need first

// Note: we can achieve the same effect by calling ModifyTaskByID directly inside
// the process of 'ADMIN ALTER DDL JOB xxx', so we can eliminate the goroutine,
// but if the task hasn't been created we need to make sure the task is created
// with config after ALTER DDL JOB is executed. A possible solution is to make
// the DXF task submission and 'ADMIN ALTER DDL JOB xxx' txn conflict with each
// other when they overlap in time, by modify the job at the same time when submit
// task, as we are using optimistic txn. But this will cause WRITE CONFLICT with
// outer txn in transitOneJobStep.

remove some useless code such as skipWriteBinlog
some refactor such as create static errors using errors.New of golang to avoid stack overhead of pingcap/errors and replace error equal check with goerrors.Is

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 · 2025-01-20T13:55:31Z

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

codecov · 2025-01-20T14:22:53Z

Codecov Report

Attention: Patch coverage is 88.82353% with 19 lines in your changes missing coverage. Please review.

Project coverage is 75.7585%. Comparing base (444c38f) to head (6c7230a).
Report is 61 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #59046        +/-   ##
================================================
+ Coverage   73.0574%   75.7585%   +2.7010%     
================================================
  Files          1689       1743        +54     
  Lines        466990     485676     +18686     
================================================
+ Hits         341171     367941     +26770     
+ Misses       104844      95371      -9473     
- Partials      20975      22364      +1389

Flag	Coverage Δ
integration	`49.8536% <10.2941%> (?)`
unit	`73.0140% <88.8235%> (+0.7559%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`64.2598% <ø> (+18.8511%)`	⬆️

D3Hunter · 2025-01-21T07:05:26Z

/retest

tiprow · 2025-01-21T07:05:58Z

@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

D3Hunter · 2025-01-21T08:28:09Z

/retest

tiprow · 2025-01-21T08:28:31Z

@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

…te-dxf-dynamic-param

D3Hunter · 2025-01-22T05:52:22Z

/retest

tiprow · 2025-01-22T05:52:45Z

@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

…te-dxf-dynamic-param

D3Hunter · 2025-02-07T08:11:37Z

@lance6716 @tangenta

lance6716

7 / 19 files viewed

D3Hunter · 2025-02-11T05:45:05Z

@lance6716 ptal

lance6716

rest lgtm

lance6716 · 2025-02-11T07:32:48Z

pkg/ddl/index.go

 	err = g.Wait()
 	return err
 }

+// Note: we can achieve the same effect by calling ModifyTaskByID directly inside


I didn't review most of DXF codes so I'm not sure what's the meaning od this function comment. Just to check:

it says we only allow some functions to write to the system table of DXF, especially, we don't allow ADMIN ALTER DDL JOB to write because it's heavier to rollback on conflict?

Can you explain in this design, which functions are allowed to write the system table? I want to check if they still can conflict with each other.

lance6716 · 2025-02-11T07:49:48Z

pkg/ddl/index.go

+				zap.String("state", currTask.State.String()))
+			continue
+		}
+		if err = taskManager.ModifyTaskByID(ctx, taskID, &proto.ModifyParam{


when checking the implementation of ModifyTaskByID I see the "old values" are not added to the UPDATE .. WHERE clause. So double check that it is expected for two concurrent call of ModifyTaskByID, the final task arguments can be updated as any of the call?

ModifyTaskByID will transit the task into modifying state if it's in the prev state, so only 1 will success for concurrent calls

lance6716 · 2025-02-11T07:52:35Z

pkg/ddl/index.go

+	if err != nil {
+		return 0, err
+	}
+	return min(workerCnt, cpuCount), nil


Can you explain the reason why we limit the concurrency? If the work is not a pure compute workload, increase the concurrency to more that CPU count can make use of IO time.

only abstract logic in here https://github.com/pingcap/tidb/pull/59046/files#diff-45ad039a8b87835b475a52615355c444538d3a1f31a84d7b3ac94e15b1ce8721L2550-L2554

valid range of tidb_ddl_reorg_worker_cnt is [1, 256], but for a DXF task, valid range of task concurrency or slots is [1, cpuCount]

D3Hunter · 2025-02-14T06:20:49Z

ping @lance6716

tangenta · 2025-02-17T03:35:24Z

pkg/ddl/index.go

+	jobID, taskID int64,
+	lastConcurrency, lastBatchSize, lastMaxWriteSpeed int,
+) {
+	logger := logutil.DDLLogger().With(zap.Int64("jobId", jobID), zap.Int64("taskId", taskID))


Suggested change

logger := logutil.DDLLogger().With(zap.Int64("jobId", jobID), zap.Int64("taskId", taskID))

logger := logutil.DDLLogger().With(zap.Int64("jobID", jobID), zap.Int64("taskID", taskID))

tangenta · 2025-02-17T03:38:46Z

pkg/ddl/index.go

+	taskManager storage.Manager,
+	done chan struct{},
+	jobID, taskID int64,
+	lastConcurrency, lastBatchSize, lastMaxWriteSpeed int,


Maybe use "old" / "new" to replace "last" / "latest"?

ti-chi-bot · 2025-02-17T03:38:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, tangenta

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~~ [lance6716,tangenta]
~~pkg/ddl/OWNERS~~ [lance6716,tangenta]
~~pkg/disttask/OWNERS~~ [lance6716,tangenta]

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-02-17T03:38:57Z

[LGTM Timeline notifier]

Timeline:

2025-02-14 07:13:53.737282322 +0000 UTC m=+599876.133504384: ☑️ agreed by lance6716.
2025-02-17 03:38:55.936594453 +0000 UTC m=+846178.332816516: ☑️ agreed by tangenta.

D3Hunter added 4 commits January 20, 2025 18:01

change

9585af5

change

f8bdf8f

change

f257d63

bazel

65664cc

ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 20, 2025

change

c774038

D3Hunter added 3 commits January 20, 2025 22:57

change

ea3b0ed

change

9ebee76

add comments

8cbceff

D3Hunter changed the title ~~[WIP]ddl: part 1 of add-index integrating dxf dynamic modify param~~ ddl: part 1 of add-index integrating dxf dynamic modify param Jan 21, 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 Jan 21, 2025

D3Hunter requested review from lance6716 and tangenta January 21, 2025 08:31

Merge remote-tracking branch 'upstream/master' into add-index/integra…

153496c

…te-dxf-dynamic-param

D3Hunter added 3 commits January 24, 2025 14:55

Merge remote-tracking branch 'upstream/master' into add-index/integra…

f9ca8a0

…te-dxf-dynamic-param

Merge remote-tracking branch 'upstream/master' into add-index/integra…

b0a05a0

…te-dxf-dynamic-param

Merge remote-tracking branch 'upstream/master' into add-index/integra…

6c7230a

…te-dxf-dynamic-param

lance6716 reviewed Feb 8, 2025

View reviewed changes

lance6716 reviewed Feb 11, 2025

View reviewed changes

lance6716 approved these changes Feb 14, 2025

View reviewed changes

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

tangenta approved these changes Feb 17, 2025

View reviewed changes

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

ti-chi-bot bot merged commit baf1600 into pingcap:master Feb 17, 2025
27 checks passed

D3Hunter deleted the add-index/integrate-dxf-dynamic-param branch February 18, 2025 02:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: part 1 of add-index integrating dxf dynamic modify param #59046

ddl: part 1 of add-index integrating dxf dynamic modify param #59046

D3Hunter commented Jan 20, 2025 •

edited

Loading

tiprow bot commented Jan 20, 2025

codecov bot commented Jan 20, 2025 •

edited

Loading

D3Hunter commented Jan 21, 2025

tiprow bot commented Jan 21, 2025

D3Hunter commented Jan 21, 2025

tiprow bot commented Jan 21, 2025

D3Hunter commented Jan 22, 2025

tiprow bot commented Jan 22, 2025

D3Hunter commented Feb 7, 2025

lance6716 left a comment

D3Hunter commented Feb 11, 2025

lance6716 left a comment

lance6716 Feb 11, 2025

lance6716 Feb 11, 2025

D3Hunter Feb 11, 2025 •

edited

Loading

lance6716 Feb 11, 2025

D3Hunter Feb 11, 2025

D3Hunter commented Feb 14, 2025

tangenta Feb 17, 2025

D3Hunter Feb 18, 2025

tangenta Feb 17, 2025

ti-chi-bot bot commented Feb 17, 2025

ti-chi-bot bot commented Feb 17, 2025

	logger := logutil.DDLLogger().With(zap.Int64("jobId", jobID), zap.Int64("taskId", taskID))
	logger := logutil.DDLLogger().With(zap.Int64("jobID", jobID), zap.Int64("taskID", taskID))

ddl: part 1 of add-index integrating dxf dynamic modify param #59046

ddl: part 1 of add-index integrating dxf dynamic modify param #59046

Conversation

D3Hunter commented Jan 20, 2025 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jan 20, 2025

codecov bot commented Jan 20, 2025 • edited Loading

Codecov Report

D3Hunter commented Jan 21, 2025

tiprow bot commented Jan 21, 2025

D3Hunter commented Jan 21, 2025

tiprow bot commented Jan 21, 2025

D3Hunter commented Jan 22, 2025

tiprow bot commented Jan 22, 2025

D3Hunter commented Feb 7, 2025

lance6716 left a comment

Choose a reason for hiding this comment

D3Hunter commented Feb 11, 2025

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 Feb 11, 2025

Choose a reason for hiding this comment

lance6716 Feb 11, 2025

Choose a reason for hiding this comment

D3Hunter Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

lance6716 Feb 11, 2025

Choose a reason for hiding this comment

D3Hunter Feb 11, 2025

Choose a reason for hiding this comment

D3Hunter commented Feb 14, 2025

tangenta Feb 17, 2025

Choose a reason for hiding this comment

D3Hunter Feb 18, 2025

Choose a reason for hiding this comment

tangenta Feb 17, 2025

Choose a reason for hiding this comment

ti-chi-bot bot commented Feb 17, 2025

ti-chi-bot bot commented Feb 17, 2025

[LGTM Timeline notifier]

D3Hunter commented Jan 20, 2025 •

edited

Loading

codecov bot commented Jan 20, 2025 •

edited

Loading

D3Hunter Feb 11, 2025 •

edited

Loading