disttask, ddl: move backend ctx register code to scheduler #46952

tangenta · 2023-09-13T10:18:30Z

What problem does this PR solve?

Issue Number: ref #46258

Problem Summary:

Previously, backendCtx is registered & unregistered every time the subtasks enter the next step. This is quite unefficient because construction & destruction take times.

The previous implementation also has the following drawbacks:

It is easy to forget unregistering after a stage complete, causing resource leakage.
The context that should be reused accross multiple stages maybe unexpectedly canceled.

What is changed and how it works?

Move register code to initialization of Scheduler.
Add Close() method to Scheduler, and unregister backendCtx in it.
Code refactoring.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
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

tiprow · 2023-09-13T10:18:52Z

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

codecov · 2023-09-13T10:45:48Z

Codecov Report

Merging #46952 (88b9d5f) into master (a7e3176) will decrease coverage by 0.6305%.
Report is 14 commits behind head on master.
The diff coverage is 31.7757%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46952        +/-   ##
================================================
- Coverage   73.2988%   72.6684%   -0.6305%     
================================================
  Files          1331       1353        +22     
  Lines        397485     404404      +6919     
================================================
+ Hits         291352     293874      +2522     
- Misses        87533      91875      +4342     
- Partials      18600      18655        +55

Flag	Coverage Δ
integration	`28.8527% <0.0000%> (?)`
unit	`73.3098% <31.7757%> (+0.0109%)`	⬆️

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

Components	Coverage Δ
dumpling	`53.9263% <ø> (-0.1181%)`	⬇️
parser	`85.0019% <ø> (ø)`
br	`48.5715% <ø> (-4.3588%)`	⬇️

ddl/stage_scheduler.go

ywqzzy

Rest LGTM

D3Hunter

rest lgtm

D3Hunter · 2023-09-13T13:32:39Z

ddl/stage_scheduler.go

 		d:             d,
 	}
 	s.BaseScheduler.Extension = s
+
+	wrapErr := func(err error) scheduler.Scheduler {
+		s.BaseScheduler.Extension = &failedExtension{err: err}


maybe add a Init which returns error to scheduler and move below part into it?

it's hacky to do it using this way.

D3Hunter · 2023-09-13T13:37:44Z

ddl/stage_scheduler.go

+	if s.backendCtx != nil {
+		ingest.LitBackCtxMgr.Unregister(s.jobID)
+	}


call Close of base scheduler

D3Hunter · 2023-09-13T13:39:08Z

util/generic/sync_map.go

@@ -44,6 +44,15 @@ func (m *SyncMap[K, V]) Load(key K) (V, bool) {
 	return val, exist
 }

+// LoadAndDelete loads and deletes a key value atomically.
+func (m *SyncMap[K, V]) LoadAndDelete(key K) (V, bool) {


extend Delete to return previous value and existed?

GMHDBJD

LGTM

ti-chi-bot · 2023-09-13T15:28:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GMHDBJD, ywqzzy

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~~ [GMHDBJD]

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-13T15:29:01Z

[LGTM Timeline notifier]

Timeline:

2023-09-13 11:14:52.224901446 +0000 UTC m=+84058.192489495: ☑️ agreed by ywqzzy.
2023-09-13 15:29:00.327309551 +0000 UTC m=+99306.294897600: ☑️ agreed by GMHDBJD.

D3Hunter · 2023-09-13T15:55:36Z

/hold

D3Hunter · 2023-09-14T04:13:26Z

/unhold

hawkingrei · 2023-09-14T06:45:06Z

/ok-to-test

tangenta · 2023-09-14T07:13:01Z

/retest

disttask, ddl: move backend ctx register code to scheduler

c3fd9b6

tangenta added 2 commits September 13, 2023 18:23

fix TestManager

b4d9580

fix concurrent delete

8629b43

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

fix linter

70c1e47

tangenta changed the title ~~[WIP] disttask, ddl: move backend ctx register code to scheduler~~ disttask, ddl: move backend ctx register code to scheduler Sep 13, 2023

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 13, 2023

tangenta requested review from D3Hunter, GMHDBJD and ywqzzy September 13, 2023 10:50

ywqzzy reviewed Sep 13, 2023

View reviewed changes

ddl/stage_scheduler.go Outdated Show resolved Hide resolved

ywqzzy approved these changes Sep 13, 2023

View reviewed changes

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

ywqzzy mentioned this pull request Sep 13, 2023

Enhance distributed task execution framework #46258

Closed

76 tasks

rename stuff

964ed52

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 13, 2023

rename stuff

7848b5d

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 13, 2023

fix TestOnRunnableTasks

cf932e3

D3Hunter reviewed Sep 13, 2023

View reviewed changes

GMHDBJD approved these changes Sep 13, 2023

View reviewed changes

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

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 13, 2023

address comments

b498374

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 14, 2023

tangenta added 2 commits September 14, 2023 13:42

fix linter

1785de5

fix typo

88b9d5f

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

ti-chi-bot bot merged commit d2e39d7 into pingcap:master Sep 14, 2023
12 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disttask, ddl: move backend ctx register code to scheduler #46952

disttask, ddl: move backend ctx register code to scheduler #46952

tangenta commented Sep 13, 2023 •

edited

Loading

tiprow bot commented Sep 13, 2023

codecov bot commented Sep 13, 2023 •

edited

Loading

ywqzzy left a comment

D3Hunter left a comment

D3Hunter Sep 13, 2023

D3Hunter Sep 13, 2023

D3Hunter Sep 13, 2023

GMHDBJD left a comment

ti-chi-bot bot commented Sep 13, 2023

ti-chi-bot bot commented Sep 13, 2023

D3Hunter commented Sep 13, 2023

D3Hunter commented Sep 14, 2023

hawkingrei commented Sep 14, 2023

tangenta commented Sep 14, 2023

disttask, ddl: move backend ctx register code to scheduler #46952

disttask, ddl: move backend ctx register code to scheduler #46952

Conversation

tangenta commented Sep 13, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

tiprow bot commented Sep 13, 2023

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

ywqzzy left a comment

Choose a reason for hiding this comment

D3Hunter left a comment

Choose a reason for hiding this comment

D3Hunter Sep 13, 2023

Choose a reason for hiding this comment

D3Hunter Sep 13, 2023

Choose a reason for hiding this comment

D3Hunter Sep 13, 2023

Choose a reason for hiding this comment

GMHDBJD left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 13, 2023

ti-chi-bot bot commented Sep 13, 2023

[LGTM Timeline notifier]

D3Hunter commented Sep 13, 2023

D3Hunter commented Sep 14, 2023

hawkingrei commented Sep 14, 2023

tangenta commented Sep 14, 2023

tangenta commented Sep 13, 2023 •

edited

Loading

codecov bot commented Sep 13, 2023 •

edited

Loading