rc: add resource group name to ddl #46756

okJiang · 2023-09-07T10:49:02Z

What problem does this PR solve?

Issue Number: ref #46656

Problem Summary:

What is changed and how it works?

as title

Check List

Tests

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

add index

No code

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

wuhuizuo · 2023-09-07T11:32:49Z

/test ?

ti-chi-bot · 2023-09-07T11:32:52Z

@wuhuizuo: The following commands are available to trigger required jobs:

/test build
/test canary-scan-security
/test check-dev
/test check-dev2
/test mysql-test
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-br-integration-test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-ddl-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-prisma-test
/test pull-mysql-connector-test
/test pull-sqllogic-test
/test pull-tiflash-test
/test unit-test

The following commands are available to trigger optional jobs:

/test pull-notify-when-compatibility-sections-changed

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

In response to this:

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

tiprow · 2023-09-07T11:33:14Z

@wuhuizuo: The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

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

…ource-group

codecov · 2023-09-08T04:13:57Z

Codecov Report

Merging #46756 (d3ad2bf) into master (ae442ad) will decrease coverage by 0.1009%.
Report is 19 commits behind head on master.
The diff coverage is 93.3333%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46756        +/-   ##
================================================
- Coverage   73.3573%   73.2564%   -0.1009%     
================================================
  Files          1324       1349        +25     
  Lines        396657     406569      +9912     
================================================
+ Hits         290977     297838      +6861     
- Misses        87119      90266      +3147     
+ Partials      18561      18465        -96

Flag	Coverage Δ
integration	`27.9922% <62.6666%> (?)`
unit	`73.8778% <93.3333%> (+0.5204%)`	⬆️

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

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`85.4060% <ø> (+0.4451%)`	⬆️
br	`48.7919% <ø> (-3.8439%)`	⬇️

zimulala · 2023-09-08T06:22:12Z

ddl/ddl_worker.go

 	if tagger := w.getResourceGroupTaggerForTopSQL(job.ID); tagger != nil {
 		txn.SetOption(kv.ResourceGroupTagger, tagger)
 	}
+	if job.ReorgMeta != nil {


Could we remove this logical? It seems we do it in the funcion of
jobContext on the line767(https://github.com/pingcap/tidb/pull/46756/files#diff-a185df836b8926c4afe351d4def1b56d1e282c65aabe6677c2fa2c54590cc4b2R477)

ddl/ddl_worker.go

okJiang · 2023-09-11T04:09:45Z

/ok-to-test

zimulala · 2023-09-11T06:03:39Z

ddl/backfilling.go

@@ -684,7 +684,7 @@ func (dc *ddlCtx) writePhysicalTableRecord(sessPool *sess.Pool, t table.Physical
 		}
 	})

-	jc := dc.jobContext(job.ID)
+	jc := dc.jobContext(job.ID, job.ReorgMeta)


Why not use reorgInfo.NewJobContext() instead of this? In addition, how to determine whether to use 'reorgInfo.NewJobContext()' or 'dc.jobContext(job.ID, job.ReorgMeta)'

reorgInfo is a concept at the higher level of DDL job, in this function, we can only get DDL job. If we can get reorgInfo, we prefer to call NewJobContext first, because this function does not need to pass additional parameters. Otherwise, call jobContext().

But this function has the argument of reorgInfo *reorgInfo, so why not just use it?

fixed in 390b696

okJiang · 2023-09-11T06:54:02Z

/retest

…ource-group

zimulala · 2023-09-11T09:06:38Z

ddl/ddl_api.go

+			Warnings:          make(map[errors.ErrorID]*terror.Error),
+			WarningsCount:     make(map[errors.ErrorID]int64),
+			Location:          &model.TimeZoneLocation{Name: tzName, Offset: tzOffset},
+			ResourceGroupName: ctx.GetSessionVars().ResourceGroupName,


Do we need to add it to MultiSchemaChange's ReorgMeta?

Besides, could we add a NewReorgMeta to unify the assignment? This helps reduce the possibility of missing parameter assignments later on

Besides, could we add a NewReorgMeta to unify the assignment? This helps reduce the possibility of missing parameter assignments later on

lgtm

fixed all in d3ad2bf

zimulala

LGTM

glorv

LGTM

glorv · 2023-09-12T02:40:23Z

ddl/ddl.go

+	} else {
+		ctx = NewJobContext()
+	}
+	if reorgMeta != nil && len(ctx.resourceGroupName) == 0 {


Suggested change

if reorgMeta != nil && len(ctx.resourceGroupName) == 0 {

if reorgMeta != nil {

No need to check ctx's resource-group, it may be the default value default

Is default necessary? If it is, we need add a lock for it. Otherwise, it will occur data race.

Is there any case that depend on the ctx.resourceGroupName here? otherwise, should always override with reorgMeta's

ti-chi-bot · 2023-09-12T02:44:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glorv, zimulala

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~~ [glorv,zimulala]

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 · 2023-09-12T02:44:28Z

[LGTM Timeline notifier]

Timeline:

2023-09-12 01:49:58.144128598 +0000 UTC m=+320380.068684978: ☑️ agreed by zimulala.
2023-09-12 02:44:27.554418701 +0000 UTC m=+323649.478975096: ☑️ agreed by glorv.

okJiang · 2023-09-12T05:30:45Z

/retest

okJiang added 2 commits September 7, 2023 17:39

add resource group name during ddl

76ec7cf

add coprocssor

3e5a96b

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-tests-checked size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 7, 2023

Merge branch 'master' of https://github.com/pingcap/tidb into ddl-res…

b908d5d

…ource-group

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Sep 8, 2023

typo

dee5697

zimulala reviewed Sep 8, 2023

View reviewed changes

save work

bacc207

okJiang commented Sep 8, 2023

View reviewed changes

ddl/ddl_worker.go Outdated Show resolved Hide resolved

fix comment

37d96b3

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Sep 11, 2023

zimulala reviewed Sep 11, 2023

View reviewed changes

okJiang added 3 commits September 11, 2023 16:25

fix data race

e5e158d

Merge branch 'master' of https://github.com/pingcap/tidb into ddl-res…

05d3246

…ource-group

fix comment

390b696

zimulala reviewed Sep 11, 2023

View reviewed changes

fix comment

d3ad2bf

zimulala approved these changes Sep 12, 2023

View reviewed changes

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

glorv approved these changes Sep 12, 2023

View reviewed changes

ti-chi-bot bot added the lgtm label Sep 12, 2023

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 12, 2023

ti-chi-bot bot merged commit b7f0742 into pingcap:master Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rc: add resource group name to ddl #46756

rc: add resource group name to ddl #46756

okJiang commented Sep 7, 2023 •

edited

Loading

wuhuizuo commented Sep 7, 2023

ti-chi-bot bot commented Sep 7, 2023

tiprow bot commented Sep 7, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

zimulala Sep 8, 2023

okJiang commented Sep 11, 2023

zimulala Sep 11, 2023

okJiang Sep 11, 2023

zimulala Sep 11, 2023

okJiang Sep 11, 2023

okJiang commented Sep 11, 2023

zimulala Sep 11, 2023

zimulala Sep 11, 2023

okJiang Sep 11, 2023

okJiang Sep 11, 2023

zimulala left a comment

glorv left a comment

glorv Sep 12, 2023

okJiang Sep 12, 2023

glorv Sep 12, 2023

ti-chi-bot bot commented Sep 12, 2023

ti-chi-bot bot commented Sep 12, 2023

okJiang commented Sep 12, 2023

	if reorgMeta != nil && len(ctx.resourceGroupName) == 0 {
	if reorgMeta != nil {

rc: add resource group name to ddl #46756

rc: add resource group name to ddl #46756

Conversation

okJiang commented Sep 7, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

wuhuizuo commented Sep 7, 2023

ti-chi-bot bot commented Sep 7, 2023

tiprow bot commented Sep 7, 2023

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

okJiang commented Sep 11, 2023

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

okJiang commented Sep 11, 2023

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

zimulala left a comment

Choose a reason for hiding this comment

glorv left a 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

ti-chi-bot bot commented Sep 12, 2023

ti-chi-bot bot commented Sep 12, 2023

[LGTM Timeline notifier]

okJiang commented Sep 12, 2023

okJiang commented Sep 7, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading