ddl: refine some context usage #56243

lance6716 · 2024-09-24T02:47:40Z

What problem does this PR solve?

Issue Number: ref #56017 ref #54436

Problem Summary:

What changed and how does it work?

changed some functions to use correct context.

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: lance6716 <[email protected]>

tiprow · 2024-09-24T02:47:56Z

Hi @lance6716. 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 · 2024-09-24T03:01:03Z

Codecov Report

Attention: Patch coverage is 87.32394% with 9 lines in your changes missing coverage. Please review.

Project coverage is 76.3608%. Comparing base (01797cb) to head (a8d6af7).
Report is 24 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #56243        +/-   ##
================================================
+ Coverage   73.4215%   76.3608%   +2.9392%     
================================================
  Files          1619       1648        +29     
  Lines        446764     462300     +15536     
================================================
+ Hits         328021     353016     +24995     
+ Misses        98638      87718     -10920     
- Partials      20105      21566      +1461

Flag	Coverage Δ
integration	`52.0226% <61.9718%> (?)`
unit	`72.6038% <77.4647%> (+0.0978%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9478% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`63.0792% <100.0000%> (+17.5564%)`	⬆️

Signed-off-by: lance6716 <[email protected]>

pkg/ddl/backfilling_operators.go

Signed-off-by: lance6716 <[email protected]>

D3Hunter

20/27

D3Hunter · 2024-09-26T07:54:09Z

pkg/ddl/partition.go

@@ -2738,7 +2738,7 @@ func (w *worker) onExchangeTablePartition(jobCtx *jobContext, t *meta.Meta, job
 				zap.Stringer("job", job), zap.Int64("defID", defID), zap.Int64("partDef.ID", partDef.ID))
 			job.Args[0] = partDef.ID
 			defID = partDef.ID
-			err = updateDDLJob2Table(w.sess, job, true)
+			err = updateDDLJob2Table(w.ctx, w.sess, job, true)


use context from jobCtx

below too

D3Hunter · 2024-09-26T07:58:13Z

pkg/ddl/backfilling_operators.go

 	return &OperatorCtx{
 		Context: opCtx,
 		cancel:  cancel,
-	}
+	}, cancel


why not just use OperatorCtx.Cancel?

the unused variable (cancel) can remind the caller. This PR fixes some functions that forget to cancel, like line 678 in pkg/ddl/backfilling.go in the old code.

I doubt the remind part, coders can simple ignore it as OperatorCtx already has it

We should unify the place to call cancel, not scattering it all around

The developers have forgotten to call OperatorCtx.Cancel at line 678 in pkg/ddl/backfilling.go (there's also a missing in unit test files), so I prefer to use golang's built-in detect of unused variables to prevent future bug.

static checks doesn't help those logic bugs, but ok anyway

tests/realtikvtest/addindextest3/operator_test.go

Signed-off-by: lance6716 <[email protected]>

D3Hunter

rest lgtm

D3Hunter · 2024-09-26T11:39:02Z

pkg/ddl/backfilling.go

+
+	// spawn a goroutine to check if the job is cancelled
+	// TODO: design a more systematic way to cancel DDL jobs
+	var wg sync.WaitGroup


use our WaitGroup wrapper

D3Hunter · 2024-09-26T11:44:51Z

pkg/ddl/backfilling.go

+		}
+		defer sessPool.Put(sctx)
+
+		ticker := time.NewTicker(time.Second)


maybe reduce reorgWaitTime, we will check it in worker's runJobStep loop anyway

tidb/pkg/ddl/reorg.go

Lines 403 to 404 in 9dff38b

waitTimeout := ReorgWaitTimeout

// wait reorganization job done or timeout

Signed-off-by: lance6716 <[email protected]>

lance6716 · 2024-09-27T07:12:06Z

/unhold

ti-chi-bot · 2024-09-27T08:12:20Z

[LGTM Timeline notifier]

Timeline:

2024-09-26 07:36:10.06156099 +0000 UTC m=+1724239.801984926: ☑️ agreed by tangenta.
2024-09-27 08:12:19.534516229 +0000 UTC m=+1812809.274940167: ☑️ agreed by D3Hunter.

lance6716 · 2024-09-27T08:22:03Z

/retest

tiprow · 2024-09-27T08:22:25Z

@lance6716: 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-27T09:19:49Z

/retest

tiprow · 2024-09-27T09:20:10Z

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

ti-chi-bot · 2024-09-27T10:00:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, tangenta, YuJuncen

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~~ [D3Hunter,YuJuncen,tangenta]
~~br/OWNERS~~ [YuJuncen]
~~pkg/ddl/OWNERS~~ [D3Hunter,tangenta]
~~pkg/lightning/OWNERS~~ [D3Hunter]

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-27T11:43:36Z

@lance6716: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-br-integration-test	`a8d6af7`	link	unknown	`/test pull-br-integration-test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

lance6716 · 2024-09-27T11:56:56Z

/retest

tiprow · 2024-09-27T11:57:17Z

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

ddl: poll job cancel status and cancel ctx

5cfc85a

Signed-off-by: lance6716 <[email protected]>

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Sep 24, 2024

add part of ctx

95e4919

Signed-off-by: lance6716 <[email protected]>

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 24, 2024

finish?

b71cb87

Signed-off-by: lance6716 <[email protected]>

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 24, 2024

lance6716 changed the title ~~[WIP]ddl: poll job cancel status and cancel ctx~~ ddl: poll job cancel status and cancel ctx Sep 24, 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 24, 2024

lance6716 changed the title ~~ddl: poll job cancel status and cancel ctx~~ ddl: poll job state and cancel ctx Sep 24, 2024

lance6716 changed the title ~~ddl: poll job state and cancel ctx~~ ddl: poll job state and cancel ctx for ingest in background Sep 24, 2024

lance6716 added 4 commits September 24, 2024 15:32

fix CI

5b81a83

Signed-off-by: lance6716 <[email protected]>

fix CI

82a0780

Signed-off-by: lance6716 <[email protected]>

fix

f2bd0db

Signed-off-by: lance6716 <[email protected]>

fix CI

7981c0b

Signed-off-by: lance6716 <[email protected]>

tangenta approved these changes Sep 26, 2024

View reviewed changes

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

tangenta reviewed Sep 26, 2024

View reviewed changes

pkg/ddl/backfilling_operators.go Show resolved Hide resolved

lance6716 added 2 commits September 26, 2024 15:54

Merge branch 'master' of github.com:pingcap/tidb into ddl-cancel-ctx

da5ef91

Signed-off-by: lance6716 <[email protected]>

address comment

2732aee

Signed-off-by: lance6716 <[email protected]>

D3Hunter reviewed Sep 26, 2024

View reviewed changes

tangenta reviewed Sep 26, 2024

View reviewed changes

tests/realtikvtest/addindextest3/operator_test.go Outdated Show resolved Hide resolved

address comment

c4d7bdf

Signed-off-by: lance6716 <[email protected]>

D3Hunter reviewed Sep 26, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

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

lance6716 changed the title ~~ddl: poll job state and cancel ctx for ingest in background~~ [WIP]ddl: poll job state and cancel ctx in background Sep 27, 2024

ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2024

lance6716 added 2 commits September 27, 2024 15:07

revert some change

cd7e69e

Signed-off-by: lance6716 <[email protected]>

address comment

a8d6af7

Signed-off-by: lance6716 <[email protected]>

lance6716 changed the title ~~[WIP]ddl: poll job state and cancel ctx in background~~ ddl: refine some context usage Sep 27, 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 27, 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 27, 2024

D3Hunter approved these changes Sep 27, 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 27, 2024

YuJuncen approved these changes Sep 27, 2024

View reviewed changes

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

ti-chi-bot bot merged commit bad2ecd into pingcap:master Sep 27, 2024
32 of 33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: refine some context usage #56243

ddl: refine some context usage #56243

lance6716 commented Sep 24, 2024 •

edited

Loading

tiprow bot commented Sep 24, 2024

codecov bot commented Sep 24, 2024 •

edited

Loading

D3Hunter left a comment

D3Hunter Sep 26, 2024

D3Hunter Sep 26, 2024

lance6716 Sep 26, 2024 •

edited

Loading

D3Hunter Sep 26, 2024

lance6716 Sep 26, 2024 •

edited

Loading

D3Hunter Sep 26, 2024

D3Hunter left a comment

D3Hunter Sep 26, 2024

D3Hunter Sep 26, 2024

This comment was marked as outdated.

lance6716 commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

lance6716 commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

lance6716 commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

lance6716 commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

	waitTimeout := ReorgWaitTimeout
	// wait reorganization job done or timeout

ddl: refine some context usage #56243

ddl: refine some context usage #56243

Conversation

lance6716 commented Sep 24, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Sep 24, 2024

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

D3Hunter left a comment

Choose a reason for hiding this comment

D3Hunter Sep 26, 2024

Choose a reason for hiding this comment

D3Hunter Sep 26, 2024

Choose a reason for hiding this comment

lance6716 Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

D3Hunter Sep 26, 2024

Choose a reason for hiding this comment

lance6716 Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

D3Hunter Sep 26, 2024

Choose a reason for hiding this comment

D3Hunter left a comment

Choose a reason for hiding this comment

D3Hunter Sep 26, 2024

Choose a reason for hiding this comment

D3Hunter Sep 26, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

lance6716 commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

[LGTM Timeline notifier]

lance6716 commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

lance6716 commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

ti-chi-bot bot commented Sep 27, 2024

lance6716 commented Sep 27, 2024

tiprow bot commented Sep 27, 2024

lance6716 commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

lance6716 Sep 26, 2024 •

edited

Loading

lance6716 Sep 26, 2024 •

edited

Loading