ddl: remove `mock.Context` from DDL reorg #55873

lcwangchao · 2024-09-05T06:54:52Z

What problem does this PR solve?

Issue Number: ref #53388

What changed and how does it work?

This PR:

Introduces reorgTableMutateContext as the table.MutateContext to provide context for DDL reorg
remove newReorgSessCtx from production code.

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-09-05T06:55:08Z

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

lcwangchao · 2024-09-05T06:55:54Z

pkg/ddl/column.go

-	sessCtx := bCtx.sessCtx
-	sessCtx.GetSessionVars().StmtCtx.SetTypeFlags(
-		sessCtx.GetSessionVars().StmtCtx.TypeFlags().
-			WithIgnoreZeroDateErr(!reorgInfo.ReorgMeta.SQLMode.HasStrictMode()))
-	bCtx.exprCtx = bCtx.sessCtx.GetExprCtx()
-	bCtx.tblCtx = bCtx.sessCtx.GetTableCtx()
-


These logic removed to https://github.com/pingcap/tidb/pull/55873/files#diff-305565dd3afc50e1e61c8372fe0be0999d3f1ee171148d10232a9f1b36a39874R177-R190

lcwangchao · 2024-09-05T06:57:10Z

pkg/ddl/reorg.go

@@ -148,17 +276,6 @@ func reorgTimeZoneWithTzLoc(tzLoc *model.TimeZoneLocation) (*time.Location, erro
 	return tzLoc.GetLocation()
 }

-func newReorgSessCtx(store kv.Storage) sessionctx.Context {


Remove newReorgSessCtx from production code because it only used in test now. So we moved it to test and renamed it to newMockReorgSessCtx

lcwangchao · 2024-09-05T07:01:35Z

pkg/ddl/backfilling_scheduler.go

-	// Prevent initializing the mock context in the workers concurrently.
-	// For details, see https://github.com/pingcap/tidb/issues/40879.
-	if _, ok := sessCtx.(*mock.Context); ok {
-		_ = sessCtx.GetDomainInfoSchema()
-	}


Remove it because mock.Context is not used now.

lcwangchao · 2024-09-05T07:02:43Z

pkg/ddl/backfilling.go

 		return nil, errors.Trace(err)
 	}

+	if isUpdateColumn {


These logic is from: https://github.com/pingcap/tidb/pull/55873/files#diff-f33325470a81ba90410cdde792a53e6f6a1af0ec43baa840164a37dd9d2739a0L614-L619

codecov · 2024-09-05T07:16:20Z

Codecov Report

Attention: Patch coverage is 94.84536% with 5 lines in your changes missing coverage. Please review.

Project coverage is 57.0061%. Comparing base (fef43c5) to head (3b433b2).
Report is 19 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #55873         +/-   ##
=================================================
- Coverage   72.8403%   57.0061%   -15.8342%     
=================================================
  Files          1599       1754        +155     
  Lines        444670     633551     +188881     
=================================================
+ Hits         323899     361163      +37264     
- Misses       100835     247519     +146684     
- Partials      19936      24869       +4933

Flag	Coverage Δ
integration	`39.5038% <58.7628%> (?)`
unit	`72.0372% <94.8453%> (+0.0751%)`	⬆️

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

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

lcwangchao · 2024-09-05T09:22:35Z

/retest

tiprow · 2024-09-05T09:22:56Z

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

lcwangchao · 2024-09-05T14:39:24Z

/retest

tiprow · 2024-09-05T14:39:57Z

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

pkg/ddl/reorg.go

YangKeao

LGTM

ti-chi-bot · 2024-09-09T05:48:13Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/ddl/OWNERS~~ [tangenta]
~~pkg/table/OWNERS~~ [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 · 2024-09-09T05:48:18Z

[LGTM Timeline notifier]

Timeline:

2024-09-09 02:53:44.3614736 +0000 UTC m=+238494.101897540: ☑️ agreed by tangenta.
2024-09-09 05:48:17.203196688 +0000 UTC m=+248966.943620627: ☑️ agreed by YangKeao.

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 5, 2024

lcwangchao commented Sep 5, 2024

View reviewed changes

lcwangchao force-pushed the ddltblctx branch from a53b568 to 2f55b48 Compare September 5, 2024 07:00

lcwangchao commented Sep 5, 2024

View reviewed changes

lcwangchao force-pushed the ddltblctx branch from 2f55b48 to c62aeb6 Compare September 5, 2024 08:26

lcwangchao force-pushed the ddltblctx branch from c62aeb6 to 7c7458b Compare September 5, 2024 12:16

ddl: remove mock.Context from DDL reorg

3cf83be

lcwangchao force-pushed the ddltblctx branch from 7c7458b to 3cf83be Compare September 5, 2024 13:32

tangenta reviewed Sep 6, 2024

View reviewed changes

pkg/ddl/reorg.go Show resolved Hide resolved

add some comment

3b433b2

lcwangchao force-pushed the ddltblctx branch from 3ca44c5 to 3b433b2 Compare September 6, 2024 15:29

tangenta approved these changes Sep 9, 2024

View reviewed changes

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

YangKeao 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

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

lcwangchao deleted the ddltblctx branch September 9, 2024 08:34

This was referenced Sep 12, 2024

remove mock.Context usage in production code #53388

Closed

alter table modify column to a time type may generate incorrect value #56051

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: remove `mock.Context` from DDL reorg #55873

ddl: remove `mock.Context` from DDL reorg #55873

lcwangchao commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lcwangchao Sep 5, 2024

lcwangchao Sep 5, 2024

lcwangchao Sep 5, 2024

lcwangchao Sep 5, 2024

codecov bot commented Sep 5, 2024 •

edited

Loading

lcwangchao commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lcwangchao commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

YangKeao left a comment

ti-chi-bot bot commented Sep 9, 2024

ti-chi-bot bot commented Sep 9, 2024

ddl: remove mock.Context from DDL reorg #55873

ddl: remove mock.Context from DDL reorg #55873

Conversation

lcwangchao commented Sep 5, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Sep 5, 2024

lcwangchao Sep 5, 2024

Choose a reason for hiding this comment

lcwangchao Sep 5, 2024

Choose a reason for hiding this comment

lcwangchao Sep 5, 2024

Choose a reason for hiding this comment

lcwangchao Sep 5, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

lcwangchao commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

lcwangchao commented Sep 5, 2024

tiprow bot commented Sep 5, 2024

YangKeao left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 9, 2024

ti-chi-bot bot commented Sep 9, 2024

[LGTM Timeline notifier]

ddl: remove `mock.Context` from DDL reorg #55873

ddl: remove `mock.Context` from DDL reorg #55873

codecov bot commented Sep 5, 2024 •

edited

Loading