ddl: refactor job args for set-default-value/add column ddl. #56337

joccau · 2024-09-26T08:04:08Z

What problem does this PR solve?

Issue Number: Ref #53930
Ref #54436

Problem Summary:

What changed and how does it work?

Refactor the job args for follow ddl type:

ActionSetDefaultValue
ActionAddColumn

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

Signed-off-by: joccau <zak.zhao@pingcap.cn>

codecov · 2024-09-26T08:17:45Z

Codecov Report

Attention: Patch coverage is 96.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 76.2759%. Comparing base (c46e4bc) to head (7fe2576).
Report is 3 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #56337        +/-   ##
================================================
+ Coverage   73.4163%   76.2759%   +2.8596%     
================================================
  Files          1623       1645        +22     
  Lines        447906     456984      +9078     
================================================
+ Hits         328836     348569     +19733     
+ Misses        98924      86847     -12077     
- Partials      20146      21568      +1422

Flag	Coverage Δ
integration	`52.0158% <67.5000%> (?)`
unit	`72.6973% <96.2500%> (+0.1929%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`63.0723% <ø> (+17.5396%)`	⬆️

hawkingrei · 2024-09-26T09:10:39Z

/test fast_test_tiprow_ddl

ti-chi-bot · 2024-09-26T09:10:42Z

@hawkingrei: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test pull-br-integration-test
/test pull-integration-ddl-test
/test pull-lightning-integration-test
/test pull-mysql-client-test
/test unit-test

The following commands are available to trigger optional jobs:

/test canary-notify-when-compatibility-sections-changed
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-sqllogic-test
/test pull-tiflash-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test
pingcap/tidb/pull_br_integration_test
pingcap/tidb/pull_integration_ddl_test
pingcap/tidb/pull_lightning_integration_test
pingcap/tidb/pull_mysql_client_test

In response to this:

/test fast_test_tiprow_ddl

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/test-infra repository.

Signed-off-by: joccau <zak.zhao@pingcap.cn>

joechenrh · 2024-09-27T03:37:09Z

Shall we change arguments toDropColumnArgs in rollingbackAddColumn using FillArgs?

tidb/pkg/ddl/rollingback.go

Lines 187 to 210 in f2ed822

    
           func rollingbackAddColumn(jobCtx *jobContext, t *meta.Meta, job *model.Job) (ver int64, err error) { 
        
           	tblInfo, columnInfo, col, _, _, err := checkAddColumn(t, job) 
        
           	if err != nil { 
        
           		return ver, errors.Trace(err) 
        
           	} 
        
           	if columnInfo == nil { 
        
           		job.State = model.JobStateCancelled 
        
           		return ver, dbterror.ErrCancelledDDLJob 
        
           	} 
        
           	originalState := columnInfo.State 
        
           	columnInfo.State = model.StateDeleteOnly 
        
           	job.SchemaState = model.StateDeleteOnly 
        
           	job.Args = []any{col.Name} 
        
           	ver, err = updateVersionAndTableInfo(jobCtx, t, job, tblInfo, originalState != columnInfo.State) 
        
           	if err != nil { 
        
           		return ver, errors.Trace(err) 
        
           	} 
        
           	job.State = model.JobStateRollingback 
        
           	return ver, dbterror.ErrCancelledDDLJob 
        
           }

It's used here, which assumes that the arguments are filled by DropColumnArgs?

tidb/pkg/ddl/add_column.go

Lines 55 to 62 in f2ed822

    
           func onAddColumn(jobCtx *jobContext, t *meta.Meta, job *model.Job) (ver int64, err error) { 
        
           	// Handle the rolling back job. 
        
           	if job.IsRollingback() { 
        
           		ver, err = onDropColumn(jobCtx, t, job) 
        
           		if err != nil { 
        
           			return ver, errors.Trace(err) 
        
           		} 
        
           		return ver, nil

pkg/meta/model/job_args.go

joechenrh · 2024-09-27T03:50:27Z

pkg/meta/model/job_args.go

+type AddColumnArgs struct {
+	Col         *ColumnInfo         `json:"column_info,omitempty"`
+	Pos         *ast.ColumnPosition `json:"position,omitempty"`
+	Offset      int                 `json:"offset,omitempty"`


Seems like Offset is never used. Just remove it and put zero in fillJob?

D3Hunter · 2024-09-27T03:56:22Z

Shall we change arguments toDropColumnArgs in rollingbackAddColumn using FillArgs?

tidb/pkg/ddl/rollingback.go

Lines 187 to 210 in f2ed822

func rollingbackAddColumn(jobCtx *jobContext, t *meta.Meta, job *model.Job) (ver int64, err error) {

tblInfo, columnInfo, col, _, _, err := checkAddColumn(t, job)

if err != nil {

return ver, errors.Trace(err)

}

if columnInfo == nil {

job.State = model.JobStateCancelled

return ver, dbterror.ErrCancelledDDLJob

}

originalState := columnInfo.State

columnInfo.State = model.StateDeleteOnly

job.SchemaState = model.StateDeleteOnly

job.Args = []any{col.Name}

ver, err = updateVersionAndTableInfo(jobCtx, t, job, tblInfo, originalState != columnInfo.State)

if err != nil {

return ver, errors.Trace(err)

}

job.State = model.JobStateRollingback

return ver, dbterror.ErrCancelledDDLJob

}

It's used here, which assumes that the arguments are filled by DropColumnArgs?

tidb/pkg/ddl/add_column.go

Lines 55 to 62 in f2ed822

func onAddColumn(jobCtx *jobContext, t *meta.Meta, job *model.Job) (ver int64, err error) {

// Handle the rolling back job.

if job.IsRollingback() {

ver, err = onDropColumn(jobCtx, t, job)

if err != nil {

return ver, errors.Trace(err)

}

return ver, nil

CI passes, seems it's not covered, too bad

the case is similar to how AddPartition is rollbacked

joccau · 2024-09-27T03:56:32Z

/hold

code need change

D3Hunter · 2024-09-30T06:30:58Z

/test fast_test_tiprow_ddlargsv1

ti-chi-bot · 2024-09-30T06:31:01Z

@D3Hunter: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test pull-br-integration-test
/test pull-integration-ddl-test
/test pull-lightning-integration-test
/test pull-mysql-client-test
/test unit-test

The following commands are available to trigger optional jobs:

/test pingcap/tidb/canary_ghpr_unit_test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-sqllogic-test
/test pull-tiflash-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test
pingcap/tidb/pull_br_integration_test
pingcap/tidb/pull_integration_ddl_test
pingcap/tidb/pull_lightning_integration_test
pingcap/tidb/pull_mysql_client_test

In response to this:

/test fast_test_tiprow_ddlargsv1

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/test-infra repository.

D3Hunter · 2024-09-30T06:36:47Z

/test pull-unit-test-ddlv1

tiprow · 2024-09-30T06:37:01Z

@D3Hunter: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test pull-unit-test-ddlv1

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-30T07:00:39Z

/test pull-unit-test-ddlv1

tiprow · 2024-09-30T07:00:46Z

@joccau: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test pull-unit-test-ddlv1

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-30T07:00:49Z

/test pull-br-integration-test

tiprow · 2024-09-30T07:00:58Z

@joccau: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test pull-br-integration-test

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-30T07:01:06Z

/test fast_test_tiprow

ti-chi-bot · 2024-09-30T07:01:10Z

@joccau: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test pull-br-integration-test
/test pull-integration-ddl-test
/test pull-lightning-integration-test
/test pull-mysql-client-test
/test unit-test

The following commands are available to trigger optional jobs:

/test pingcap/tidb/canary_ghpr_unit_test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-sqllogic-test
/test pull-tiflash-test
/test pull-unit-test-ddlv1

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test
pingcap/tidb/pull_br_integration_test
pingcap/tidb/pull_integration_ddl_test
pingcap/tidb/pull_lightning_integration_test
pingcap/tidb/pull_mysql_client_test

In response to this:

/test fast_test_tiprow

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/test-infra repository.

…ltValue-bk

D3Hunter · 2024-09-30T07:21:57Z

/test fast_test_tiprow_ddlargsv1
/test pull-unit-test-ddlv1

tiprow · 2024-09-30T07:22:18Z

@D3Hunter: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test fast_test_tiprow_ddlargsv1
/test pull-unit-test-ddlv1

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-30T07:23:02Z

/test fast_test_tiprow_ddlargsv1

ti-chi-bot · 2024-09-30T07:23:05Z

@D3Hunter: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test pull-br-integration-test
/test pull-integration-ddl-test
/test pull-lightning-integration-test
/test pull-mysql-client-test
/test unit-test

The following commands are available to trigger optional jobs:

/test pingcap/tidb/canary_ghpr_unit_test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-sqllogic-test
/test pull-tiflash-test
/test pull-unit-test-ddlv1

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test
pingcap/tidb/pull_br_integration_test
pingcap/tidb/pull_integration_ddl_test
pingcap/tidb/pull_lightning_integration_test
pingcap/tidb/pull_mysql_client_test

In response to this:

/test fast_test_tiprow_ddlargsv1

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/test-infra repository.

D3Hunter · 2024-09-30T07:48:38Z

/hold

to wait V1 pass

D3Hunter · 2024-09-30T08:34:11Z

/retest

D3Hunter · 2024-09-30T08:34:26Z

/test fast_test_tiprow_ddlargsv1

ti-chi-bot · 2024-09-30T08:34:30Z

@D3Hunter: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test pull-br-integration-test
/test pull-integration-ddl-test
/test pull-lightning-integration-test
/test pull-mysql-client-test
/test unit-test

The following commands are available to trigger optional jobs:

/test pingcap/tidb/canary_ghpr_unit_test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-sqllogic-test
/test pull-tiflash-test
/test pull-unit-test-ddlv1

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test
pingcap/tidb/pull_br_integration_test
pingcap/tidb/pull_integration_ddl_test
pingcap/tidb/pull_lightning_integration_test
pingcap/tidb/pull_mysql_client_test

In response to this:

/test fast_test_tiprow_ddlargsv1

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/test-infra repository.

joccau · 2024-09-30T08:41:15Z

/unhold

D3Hunter · 2024-09-30T08:45:17Z

/hold

D3Hunter · 2024-09-30T09:06:38Z

/unhold

v1 pass https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fpull_unit_test_ddlv1/detail/pull_unit_test_ddlv1/4/pipeline

D3Hunter · 2024-09-30T09:23:32Z

/test pull-unit-test-ddlv1

tiprow · 2024-09-30T09:23:47Z

@D3Hunter: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test pull-unit-test-ddlv1

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.

refactor job args for ActionSetDefaultValue

8337779

Signed-off-by: joccau <zak.zhao@pingcap.cn>

joccau changed the title ~~ddl: refactor job args for setd-default-value ddl.~~ ddl: refactor job args for set-default-value ddl. Sep 26, 2024

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

joccau changed the title ~~ddl: refactor job args for set-default-value ddl.~~ ddl: refactor job args for set-default-value/add column ddl. Sep 26, 2024

joccau force-pushed the refactor-SetDefaultValue-bk branch from 272174c to 918300a Compare September 26, 2024 09:21

refactor the job args for add column ddl.

9a4153a

Signed-off-by: joccau <zak.zhao@pingcap.cn>

joccau force-pushed the refactor-SetDefaultValue-bk branch from 918300a to 9a4153a Compare September 26, 2024 09:29

joccau requested review from D3Hunter and lance6716 September 27, 2024 00:38

D3Hunter previously approved these changes Sep 27, 2024

View reviewed changes

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

joechenrh reviewed Sep 27, 2024

View reviewed changes

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

joechenrh reviewed Sep 27, 2024

View reviewed changes

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2024

ti-chi-bot bot removed the approved label Sep 27, 2024

Merge remote-tracking branch 'upstream/master' into refactor-SetDefau…

7fe2576

…ltValue-bk

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2024

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

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2024

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

ti-chi-bot bot merged commit 72f067b into pingcap:master Sep 30, 2024
27 checks passed

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: refactor job args for set-default-value/add column ddl. #56337

ddl: refactor job args for set-default-value/add column ddl. #56337

joccau commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

hawkingrei commented Sep 26, 2024

ti-chi-bot bot commented Sep 26, 2024

joechenrh commented Sep 27, 2024 •

edited

Loading

joechenrh Sep 27, 2024 •

edited

Loading

D3Hunter commented Sep 27, 2024

joccau commented Sep 27, 2024

D3Hunter commented Sep 30, 2024

ti-chi-bot bot commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

joccau commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

joccau commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

joccau commented Sep 30, 2024

ti-chi-bot bot commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

ti-chi-bot bot commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

ti-chi-bot bot commented Sep 30, 2024

joccau commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

ddl: refactor job args for set-default-value/add column ddl. #56337

ddl: refactor job args for set-default-value/add column ddl. #56337

Conversation

joccau commented Sep 26, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

hawkingrei commented Sep 26, 2024

ti-chi-bot bot commented Sep 26, 2024

joechenrh commented Sep 27, 2024 • edited Loading

joechenrh Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

D3Hunter commented Sep 27, 2024

joccau commented Sep 27, 2024

D3Hunter commented Sep 30, 2024

ti-chi-bot bot commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

joccau commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

joccau commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

joccau commented Sep 30, 2024

ti-chi-bot bot commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

ti-chi-bot bot commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

ti-chi-bot bot commented Sep 30, 2024

joccau commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

D3Hunter commented Sep 30, 2024

tiprow bot commented Sep 30, 2024

joccau commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

joechenrh commented Sep 27, 2024 •

edited

Loading

joechenrh Sep 27, 2024 •

edited

Loading