ddl: add job version 2 and use typed args for truncate table #55854

D3Hunter · 2024-09-04T11:32:56Z

What problem does this PR solve?

Issue Number: ref #53930

Problem Summary:

What changed and how does it work?

add DDL job version 2 to use typed struct to replace old un-typed args slice
add new typed args for TruncateTable
currently we choose job version for new jobs randomly in test, see ddl.Start. Note: this only works for truncate-table for now.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

manually run TruncateTable tests using job version 2, success

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-09-04T11:33:13Z

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.

D3Hunter · 2024-09-04T11:35:30Z

pkg/meta/model/job.go

@@ -289,14 +329,14 @@ type Job struct {
 	Query      string       `json:"query"`
 	BinlogInfo *HistoryInfo `json:"binlog"`

-	// Version indicates the DDL job version. For old jobs, it will be 0.


since version v1.0.0, the job version is already 1, so there shouldn't be jobs of version 0 now. see https://github.com/pingcap/tidb/pull/3993/files#diff-a185df836b8926c4afe351d4def1b56d1e282c65aabe6677c2fa2c54590cc4b2R44-R45

codecov · 2024-09-04T11:57:19Z

Codecov Report

Attention: Patch coverage is 81.43460% with 44 lines in your changes missing coverage. Please review.

Project coverage is 57.1298%. Comparing base (e9124dd) to head (c0530a1).
Report is 24 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #55854         +/-   ##
=================================================
- Coverage   72.8514%   57.1298%   -15.7217%     
=================================================
  Files          1601       1756        +155     
  Lines        445302     633207     +187905     
=================================================
+ Hits         324409     361750      +37341     
- Misses       100894     246610     +145716     
- Partials      19999      24847       +4848

Flag	Coverage Δ
integration	`39.6920% <69.1983%> (?)`
unit	`72.1897% <81.4346%> (+0.2285%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9567% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`62.9537% <ø> (+17.1771%)`	⬆️

…name

D3Hunter · 2024-09-05T17:12:05Z

/retest

tiprow · 2024-09-05T17:12:32Z

@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 · 2024-09-05T17:17:07Z

/retest

tiprow · 2024-09-05T17:17:29Z

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

joccau · 2024-09-06T03:34:38Z

Maybe we could abstract some function, like this:

func setArgsForTruncateTable(job *model.job,  actualArgs *model.TruncateTableArgs, v version) {
    if v == 0 {
        // set args with the actualArgs.
        job.Args[0] = actualArg.NewTableID            
        job.Args[1] = ...
    } else {
       // set argsV2 with actualArgs.
        job.ArgsV2 = &model.TruncateTableArgs{
			NewTableID: 2,
		}
    }
}

func getArgsForTruncateTable(job *model.job, actualArgs *model.TruncateTableArgs, v version){
    if v == 0 {
        // set actualArgs with job.args
        newTableID = job.Args[0].(int64)
    } else {
       // set actualArgs with job.argsV2
       newTableID = job.ArgsV2.(*model.TruncateTableArgs).NewTableID
    }
}

func getIDCountForTruncateTable(job *model.job, v version) int  {
	if jobW.Version == model.JobVersion1 {
		partCount := jobW.Args[3].(int)
		count += 1 + partCount
	} else {
		count += 1 + len(jobW.ArgsV2.(*model.TruncateTableArgs).OldPartitionIDs)
	}
        return count.
}

And we can call this function in DDL jobSubmit and ddl jobScheduler and ...., put together functions that modify related parameters for different DDL type.

D3Hunter · 2024-09-06T05:25:53Z

/retest

tiprow · 2024-09-06T05:26:16Z

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

lance6716 · 2024-09-06T08:43:43Z

pkg/ddl/executor.go

@@ -6426,7 +6429,12 @@ func (e *executor) DoDDLJobWrapper(ctx sessionctx.Context, jobW *JobWrapper) (re
 func HandleLockTablesOnSuccessSubmit(ctx sessionctx.Context, jobW *JobWrapper) {
 	if jobW.Type == model.ActionTruncateTable {
 		if ok, lockTp := ctx.CheckTableLocked(jobW.TableID); ok {
-			newTableID := jobW.Args[0].(int64)
+			var newTableID int64
+			if jobW.Version == model.JobVersion1 {


like GetTruncateTableArgsBeforeRun, this part can also be encapsulated?

lance6716 · 2024-09-06T08:43:53Z

pkg/ddl/executor.go

@@ -6437,7 +6445,12 @@ func HandleLockTablesOnSuccessSubmit(ctx sessionctx.Context, jobW *JobWrapper) {
 func HandleLockTablesOnFinish(ctx sessionctx.Context, jobW *JobWrapper, ddlErr error) {
 	if jobW.Type == model.ActionTruncateTable {
 		if ddlErr != nil {
-			newTableID := jobW.Args[0].(int64)
+			var newTableID int64
+			if jobW.Version == model.JobVersion1 {


lance6716 · 2024-09-06T08:46:39Z

pkg/ddl/job_scheduler.go

@@ -723,7 +723,13 @@ func job2UniqueIDs(job *model.Job, schema bool) string {
 		if schema {
 			return strconv.FormatInt(job.SchemaID, 10)
 		}
-		return strconv.FormatInt(job.TableID, 10) + "," + strconv.FormatInt(job.Args[0].(int64), 10)
+		var newTableID int64
+		if job.Version == model.JobVersion1 {


pkg/ddl/job_submitter.go

pkg/meta/model/job_args.go

D3Hunter · 2024-09-06T10:00:24Z

@lance6716 also add another API for changed args, see https://github.com/pingcap/tidb/pull/55919/files#diff-0a328bfc50df4984d8b701f6cfb588c8bc675992e32e00ae05f2d900147ea8dfR44-R46

D3Hunter · 2024-09-06T11:40:11Z

/retest

tiprow · 2024-09-06T11:40:33Z

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

lance6716 · 2024-09-09T01:53:47Z

pkg/ddl/job_submitter.go

-				partIDs := make([]int64, partCount)
-				for i := range partIDs {
-					partIDs[i] = alloc.next()
+				if jobW.Version == model.JobVersion1 {


for these getter logic, I prefer we always use a function to convert all version representations to TruncateTableArgs, then caller use TruncateTableArgs.

like

if !jobW.IDAllocated { arg := getTruncateTableArgs(job) arg.NewTableID = alloc.next() ...

for new jobs, i plan put JobArgs into JobWrapper, and fill to job on insert, so we can use it directly here.

ti-chi-bot · 2024-09-09T01:54:02Z

[LGTM Timeline notifier]

Timeline:

2024-09-06 10:29:53.410210012 +0000 UTC m=+6663.150633946: ☑️ agreed by joccau.
2024-09-09 01:54:01.385231471 +0000 UTC m=+234911.125655403: ☑️ agreed by lance6716.

ti-chi-bot · 2024-09-09T02:51:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joccau, 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~~ [joccau,lance6716,tangenta]
~~pkg/ddl/OWNERS~~ [lance6716,tangenta]
~~pkg/meta/OWNERS~~ [tangenta]

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

D3Hunter · 2024-09-09T06:25:16Z

/retest

tiprow · 2024-09-09T06:25:38Z

@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 · 2024-09-09T07:02:30Z

/retest

tiprow · 2024-09-09T07:02:53Z

@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 · 2024-09-09T07:18:17Z

/retest

tiprow · 2024-09-09T07:18:40Z

@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 · 2024-09-09T07:58:17Z

/retest

tiprow · 2024-09-09T07:58:39Z

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

use new args for truncate table

eb0584d

D3Hunter commented Sep 4, 2024

View reviewed changes

change

e8368ce

D3Hunter added 2 commits September 5, 2024 14:44

change

fc213b7

bazel

67ec37c

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 5, 2024

todo

8cdc42c

ti-chi-bot bot removed do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked labels Sep 5, 2024

D3Hunter added 2 commits September 5, 2024 16:37

fix test

345284e

choose job version randomly in test

a238153

D3Hunter changed the title ~~[WIP]ddl: add job version 2 and use typed args for truncate table~~ ddl: add job version 2 and use typed args for truncate table Sep 5, 2024

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 Sep 5, 2024

D3Hunter requested a review from lance6716 September 5, 2024 09:45

D3Hunter added 2 commits September 5, 2024 17:51

fix test

99a5d0b

Merge remote-tracking branch 'upstream/master' into typed-job-args-re…

ecb3836

…name

D3Hunter requested a review from joccau September 6, 2024 03:21

lance6716 reviewed Sep 6, 2024

View reviewed changes

joccau reviewed Sep 6, 2024

View reviewed changes

pkg/meta/model/job_args.go Show resolved Hide resolved

fix comments

c0530a1

joccau approved these changes Sep 6, 2024

View reviewed changes

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

lance6716 approved these changes Sep 9, 2024

View reviewed changes

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

tangenta approved these changes Sep 9, 2024

View reviewed changes

ti-chi-bot bot added the approved label Sep 9, 2024

tangenta mentioned this pull request Sep 9, 2024

*: support for adding/dropping vector index | tidb-test=723d2370f1b3956d22a5ab87b1fc8b9b864ca051 #55839

Merged

13 tasks

ti-chi-bot bot merged commit 72c1580 into pingcap:master Sep 9, 2024
25 checks passed

D3Hunter deleted the typed-job-args-rename branch September 9, 2024 11:24

D3Hunter mentioned this pull request Sep 14, 2024

ddl code refactor/optimize #54436

Open

54 tasks

D3Hunter mentioned this pull request Oct 14, 2024

improve the usability of DDL Job.Args #53930

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: add job version 2 and use typed args for truncate table #55854

ddl: add job version 2 and use typed args for truncate table #55854

D3Hunter commented Sep 4, 2024 •

edited

Loading

tiprow bot commented Sep 4, 2024

D3Hunter Sep 4, 2024

codecov bot commented Sep 4, 2024 •

edited

Loading

D3Hunter commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

D3Hunter commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

joccau commented Sep 6, 2024 •

edited

Loading

D3Hunter commented Sep 6, 2024

tiprow bot commented Sep 6, 2024

lance6716 Sep 6, 2024

D3Hunter Sep 6, 2024

lance6716 Sep 6, 2024

lance6716 Sep 6, 2024

D3Hunter commented Sep 6, 2024

D3Hunter commented Sep 6, 2024

tiprow bot commented Sep 6, 2024

lance6716 Sep 9, 2024

D3Hunter Sep 9, 2024

ti-chi-bot bot commented Sep 9, 2024

ti-chi-bot bot commented Sep 9, 2024

D3Hunter commented Sep 9, 2024

tiprow bot commented Sep 9, 2024

D3Hunter commented Sep 9, 2024

tiprow bot commented Sep 9, 2024

D3Hunter commented Sep 9, 2024

tiprow bot commented Sep 9, 2024

D3Hunter commented Sep 9, 2024

tiprow bot commented Sep 9, 2024

ddl: add job version 2 and use typed args for truncate table #55854

ddl: add job version 2 and use typed args for truncate table #55854

Conversation

D3Hunter commented Sep 4, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Sep 4, 2024

D3Hunter Sep 4, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 4, 2024 • edited Loading

Codecov Report

D3Hunter commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

D3Hunter commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

joccau commented Sep 6, 2024 • edited Loading

D3Hunter commented Sep 6, 2024

tiprow bot commented Sep 6, 2024

lance6716 Sep 6, 2024

Choose a reason for hiding this comment

D3Hunter Sep 6, 2024

Choose a reason for hiding this comment

lance6716 Sep 6, 2024

Choose a reason for hiding this comment

lance6716 Sep 6, 2024

Choose a reason for hiding this comment

D3Hunter commented Sep 6, 2024

D3Hunter commented Sep 6, 2024

tiprow bot commented Sep 6, 2024

lance6716 Sep 9, 2024

Choose a reason for hiding this comment

D3Hunter Sep 9, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 9, 2024

[LGTM Timeline notifier]

ti-chi-bot bot commented Sep 9, 2024

D3Hunter commented Sep 9, 2024

tiprow bot commented Sep 9, 2024

D3Hunter commented Sep 9, 2024

tiprow bot commented Sep 9, 2024

D3Hunter commented Sep 9, 2024

tiprow bot commented Sep 9, 2024

D3Hunter commented Sep 9, 2024

tiprow bot commented Sep 9, 2024

D3Hunter commented Sep 4, 2024 •

edited

Loading

codecov bot commented Sep 4, 2024 •

edited

Loading

joccau commented Sep 6, 2024 •

edited

Loading