disttask: impl slot manager in dispatcher, and check resource before start dispatcher #49195

D3Hunter · 2023-12-05T10:12:12Z

What problem does this PR solve?

Issue Number: close #49100

Problem Summary:

What changed and how does it work?

impl slotManager on dispatcher
check resource before start disaptcher

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

test test

tiprow · 2023-12-05T10:12:31Z

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

D3Hunter · 2023-12-05T10:21:52Z

pkg/disttask/framework/dispatcher/dispatcher_manager.go

 		}()
-		dm.setRunningTask(task, dispatcher)
+		metrics.UpdateMetricsForRunTask(task)


moved from setRunningTask

ywqzzy · 2023-12-05T10:23:13Z

/cc @ywqzzy

codecov · 2023-12-05T10:25:45Z

Codecov Report

Merging #49195 (ddeb124) into master (32cf9b9) will increase coverage by 0.8469%.
Report is 36 commits behind head on master.
The diff coverage is 17.4377%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #49195        +/-   ##
================================================
+ Coverage   71.0441%   71.8911%   +0.8469%     
================================================
  Files          1368       1410        +42     
  Lines        402948     418921     +15973     
================================================
+ Hits         286271     301167     +14896     
- Misses        96736      98842      +2106     
+ Partials      19941      18912      -1029

Flag	Coverage Δ
integration	`43.6281% <17.4377%> (?)`
unit	`71.0388% <ø> (-0.0053%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9663% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`47.8780% <ø> (-5.0906%)`	⬇️

okJiang · 2023-12-06T02:45:48Z

pkg/disttask/framework/dispatcher/dispatcher_manager.go

+	mu struct {
 		syncutil.RWMutex
-		taskIDs     map[int64]struct{}
 		dispatchers map[int64]Dispatcher
 	}
 }


How about using sync.Map

it's untyped. let's leave it here

okJiang

rest lgtm

okJiang · 2023-12-06T08:03:01Z

pkg/disttask/framework/dispatcher/dispatcher_manager.go

+			if taskCnt >= proto.MaxConcurrentTask {
 				break
 			}
-			for _, task := range tasks {
-				// This task is running, so no need to reprocess it.
-				if dm.isRunningTask(task.ID) {
-					continue
-				}
-				metrics.DistTaskGauge.WithLabelValues(task.Type.String(), metrics.DispatchingStatus).Inc()
-				// we check it before start dispatcher, so no need to check it again.
-				// see startDispatcher.
-				// this should not happen normally, unless user modify system table
-				// directly.
-				if getDispatcherFactory(task.Type) == nil {
-					logutil.BgLogger().Warn("unknown task type", zap.Int64("task-id", task.ID),
-						zap.Stringer("task-type", task.Type))
-					dm.failTask(task, errors.New("unknown task type"))
-					continue
-				}
-				// the task is not in runningTasks set when:
-				// owner changed or task is cancelled when status is pending.
-				if task.State == proto.TaskStateRunning || task.State == proto.TaskStateReverting || task.State == proto.TaskStateCancelling {
-					metrics.UpdateMetricsForDispatchTask(task)
-					dm.startDispatcher(task)
-					cnt++
-					continue
-				}
-				if dm.checkConcurrencyOverflow(cnt) {
-					break
-				}
-				metrics.UpdateMetricsForDispatchTask(task)
-				dm.startDispatcher(task)
-				cnt++
+			reservedExecID, ok := dm.slotMgr.canReserve(task)


why check it twice?

what's checked twice?

It is strange. In fact I commented L197 in vs code.....

I mean L197 and L158

check in L158, so no need to run any sql

okJiang · 2023-12-06T08:18:34Z

pkg/disttask/framework/dispatcher/dispatcher_test.go

+	require.Eventually(t, func() bool {
+		taskKeys := getRunningTaskKeys()
+		return err == nil && len(taskKeys) == 4 &&
+			taskKeys[0] == "key/0" && taskKeys[1] == "key/1" &&
+			taskKeys[2] == "key/3" && taskKeys[3] == "key/4"
+	}, time.Second*10, time.Millisecond*100)


Please add a comment to explain why there are 4 tasks. I think it is because the MaxDispatcherConcurency

yes, limited by max concurrent task. seems quite trivial to comment this

tangenta · 2023-12-11T05:59:40Z

pkg/disttask/framework/dispatcher/dispatcher_manager.go

+			// this should not happen normally, unless user modify system table
+			// directly.
+			if getDispatcherFactory(task.Type) == nil {


Which system table affects this behavior?

suppose user change task.Type to some invalid value

pkg/disttask/framework/dispatcher/slots.go

tangenta

rest lgtm

pkg/disttask/framework/dispatcher/slots.go

Co-authored-by: tangenta <tangenta@126.com>

ti-chi-bot · 2023-12-11T12:40:25Z

[LGTM Timeline notifier]

Timeline:

2023-12-08 07:20:47.06928148 +0000 UTC m=+1771275.734507687: ☑️ agreed by okJiang.
2023-12-11 12:40:24.102311164 +0000 UTC m=+273515.139538093: ☑️ agreed by tangenta.

ti-chi-bot · 2023-12-11T13:49:21Z

@D3Hunter: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

ti-chi-bot · 2023-12-11T13:49:27Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,okJiang,tangenta]
~~pkg/disttask/OWNERS~~ [D3Hunter,tangenta]
~~pkg/domain/OWNERS~~ [D3Hunter,tangenta]
~~pkg/executor/importer/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 · 2023-12-11T13:49:33Z

@D3Hunter: Please do not delete or edit you lgtm type comment!

In response to this:

/lgtm

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 added 10 commits December 1, 2023 17:43

change

972b6e4

change

6bf5c3c

Merge remote-tracking branch 'upstream/master' into dispatcher-slot

c28a57b

change

b833cce

test

caa7f5b

test test

test

cd6ca66

test

3714ad7

test

77cc475

bazel and lint

a893b4b

Merge remote-tracking branch 'upstream/master' into dispatcher-slot

01067cb

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 5, 2023

D3Hunter commented Dec 5, 2023

View reviewed changes

ti-chi-bot bot requested a review from ywqzzy December 5, 2023 10:23

D3Hunter added 3 commits December 5, 2023 18:41

fix test

d9331a9

bazel

1f888ea

fix flaky test

636e97b

okJiang reviewed Dec 6, 2023

View reviewed changes

okJiang approved these changes Dec 8, 2023

View reviewed changes

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

tangenta reviewed Dec 11, 2023

View reviewed changes

pkg/disttask/framework/dispatcher/slots.go Outdated Show resolved Hide resolved

tangenta reviewed Dec 11, 2023

View reviewed changes

pkg/disttask/framework/dispatcher/slots.go Outdated Show resolved Hide resolved

D3Hunter and others added 2 commits December 11, 2023 19:52

Update pkg/disttask/framework/dispatcher/slots.go

5c3745e

Co-authored-by: tangenta <tangenta@126.com>

Update pkg/disttask/framework/dispatcher/slots.go

ddeb124

Co-authored-by: tangenta <tangenta@126.com>

tangenta approved these changes Dec 11, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 11, 2023

ti-chi-bot bot added the approved label Dec 11, 2023

ti-chi-bot bot merged commit a7260ff into pingcap:master Dec 11, 2023
16 checks passed

D3Hunter deleted the dispatcher-slot branch December 11, 2023 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disttask: impl slot manager in dispatcher, and check resource before start dispatcher #49195

disttask: impl slot manager in dispatcher, and check resource before start dispatcher #49195

D3Hunter commented Dec 5, 2023

tiprow bot commented Dec 5, 2023

D3Hunter Dec 5, 2023

ywqzzy commented Dec 5, 2023

codecov bot commented Dec 5, 2023 •

edited

Loading

okJiang Dec 6, 2023

D3Hunter Dec 6, 2023

okJiang left a comment

okJiang Dec 6, 2023

D3Hunter Dec 6, 2023

okJiang Dec 6, 2023

D3Hunter Dec 7, 2023

okJiang Dec 6, 2023

D3Hunter Dec 6, 2023

tangenta Dec 11, 2023

D3Hunter Dec 11, 2023

tangenta left a comment

ti-chi-bot bot commented Dec 11, 2023

ti-chi-bot bot commented Dec 11, 2023

ti-chi-bot bot commented Dec 11, 2023

ti-chi-bot bot commented Dec 11, 2023

disttask: impl slot manager in dispatcher, and check resource before start dispatcher #49195

disttask: impl slot manager in dispatcher, and check resource before start dispatcher #49195

Conversation

D3Hunter commented Dec 5, 2023

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Dec 5, 2023

Choose a reason for hiding this comment

ywqzzy commented Dec 5, 2023

codecov bot commented Dec 5, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang 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

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

Choose a reason for hiding this comment

tangenta left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 11, 2023

[LGTM Timeline notifier]

ti-chi-bot bot commented Dec 11, 2023

ti-chi-bot bot commented Dec 11, 2023

ti-chi-bot bot commented Dec 11, 2023

codecov bot commented Dec 5, 2023 •

edited

Loading