disttask: Refactor taskExecutor slotManager to handle task occupation #49713

okJiang · 2023-12-22T06:40:58Z

What problem does this PR solve?

Issue Number: ref #49008

Problem Summary: we need free slots(resource) when higher task comes.

What changed and how does it work?

cancel the lower priority task when higher task comes, higher task is waiting slots
alloc slot to waiting task after lower task cancelled
can't alloc lower task when a task is waiting
higher task will occupy slots that the waiting task reserved, and waiting task reset

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

codecov · 2023-12-22T06:55:08Z

Codecov Report

Merging #49713 (47a7e05) into master (b1e5d61) will increase coverage by 0.2138%.
Report is 68 commits behind head on master.
The diff coverage is 5.1724%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #49713        +/-   ##
================================================
+ Coverage   70.9732%   71.1871%   +0.2138%     
================================================
  Files          1368       1430        +62     
  Lines        398095     427368     +29273     
================================================
+ Hits         282541     304231     +21690     
- Misses        95813     104068      +8255     
+ Partials      19741      19069       -672

Flag	Coverage Δ
integration	`43.8731% <5.1724%> (?)`
unit	`70.9732% <ø> (ø)`

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

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

okJiang · 2023-12-22T09:42:38Z

/cc @D3Hunter @ywqzzy @tangenta

D3Hunter · 2023-12-25T08:13:50Z

pkg/disttask/framework/taskexecutor/manager.go

@@ -218,7 +218,9 @@ func (m *Manager) onRunnableTasks(tasks []*proto.Task) {
 		}
 		logutil.Logger(m.logCtx).Info("detect new subtask", zap.Int64("task-id", task.ID))

-		if !m.slotManager.canAlloc(task) {
+		canAlloc, tasksNeedFree := m.slotManager.canAlloc(task)
+		m.onCanceledTasks(context.Background(), tasksNeedFree)


as discussed in group, this method is used to make the task as cancelled, not cancel the task-executor

also, we only need to cancel the tasks that has lower order and occupies the slots we need to run current task, not cancel all.

cancel(nil) can cancel the running task without modifying the task state.

fixed in 202ea21

D3Hunter · 2023-12-25T08:25:37Z

pkg/disttask/framework/taskexecutor/slot.go

-	// priority will be used in future
-	priority  int
-	slotCount int
+	taskWaitAlloc *proto.Task


slot manager shouldn't manage which task is waiting, and not needed

~~Do you mean manage it in tastexecutor.Manager?~~

~~Do you mean manage it in tastexecutor.Manager?~~

No need to manage task since we fetch runnableTask in loop

D3Hunter · 2023-12-25T08:31:05Z

pkg/disttask/framework/taskexecutor/slot.go

+	})
+
+	usedSlots := 0
+	for _, slotInfo := range allSlotInfos {


we should check in backward to free tasks with lowest rank

D3Hunter · 2023-12-25T08:31:24Z

pkg/disttask/framework/taskexecutor/slot.go

+		return false, nil
+	}
+
+	allSlotInfos := make([]*proto.Task, 0, len(sm.executorSlotInfos))


we can maintains this slice during alloc/free

D3Hunter · 2023-12-25T08:34:47Z

pkg/disttask/framework/taskexecutor/slot.go

+		usedSlots += slotInfo.Concurrency
+		if sm.available+usedSlots >= task.Concurrency {
+			sm.taskWaitAlloc = task
+			return false, tasksNeedFree


should return true, tasksNeedFree, to make caller know that it can alloc after freeing slots taken by those tasks

pkg/disttask/framework/taskexecutor/manager.go

Co-authored-by: D3Hunter <[email protected]>

purelind · 2023-12-26T10:27:30Z

/test all

okJiang · 2023-12-26T11:35:53Z

/test all

okJiang · 2023-12-27T02:23:37Z

/retest

ywqzzy · 2023-12-27T03:07:28Z

pkg/disttask/framework/taskexecutor/manager_test.go

+
+	// 4. task1 is released, task3 alloc success, start to run
+	ch <- context.Canceled
+	time.Sleep(time.Second)


can't rely on time.sleep to wait free()

The time.Sleep waits here

Time is unreliable, still have chance of flaky test.

Time is unreliable, still have chance of flaky test.

For what you've said, I think it needs to be analyzed on a case-by-case basis. There's obviously a waiting for checkTime, and I don't see any problem with waiting(sleep). I think what I can do is that refactor it to require.Eventually. What do you think?

pkg/disttask/framework/taskexecutor/slot.go

pkg/disttask/framework/taskexecutor/slot_test.go

D3Hunter · 2023-12-27T04:12:47Z

pkg/disttask/framework/taskexecutor/manager.go

+			break
+		}
+
+		if !canAlloc {
 			logutil.Logger(m.logCtx).Warn("subtask has been rejected", zap.Int64("task-id", task.ID))


should be debug level or remove it, to avoid print too much

pkg/disttask/framework/taskexecutor/manager.go

D3Hunter · 2023-12-27T04:23:34Z

pkg/disttask/framework/taskexecutor/manager.go

 			m.logErr(err)
 			return
 		} else if !exist {
 			continue
 		}
 		switch task.State {
 		case proto.TaskStateRunning:
+			if taskCtx.Err() != nil {


seems no need to print this

Prefer to keep it. We can know some subtasks have been canceled

there's already log at line 386

we also log in cancelTaskExecutors

updated log level to debug

just remove it

ywqzzy · 2023-12-28T07:14:32Z

pkg/disttask/framework/taskexecutor/manager.go

+	for _, task := range tasks {
+		logutil.Logger(m.logCtx).Info("cancelTasks", zap.Any("task_id", task.ID))
+		if cancel, ok := m.mu.handlingTasks[task.ID]; ok && cancel != nil {
+			// Pause all running subtasks, don't mark subtasks as canceled.


change the comments

Suggested change

// Pause all running subtasks, don't mark subtasks as canceled.

// Pause given subtasks temporarily, don't mark subtasks as canceled.

like this?

ywqzzy · 2023-12-28T07:24:59Z

pkg/disttask/framework/taskexecutor/manager.go

+			// Pause all running subtasks, don't mark subtasks as canceled.
+			// Should not change the subtask's state.
+			cancel(nil)
+		}


why not m.slotManager.free(t.ID) here?

pkg/disttask/framework/taskexecutor/manager.go

D3Hunter

rest lgtm

pkg/disttask/framework/taskexecutor/manager_test.go

pkg/disttask/framework/taskexecutor/manager.go

Co-authored-by: D3Hunter <[email protected]>

ti-chi-bot · 2023-12-28T08:38:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, 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:

~~pkg/disttask/OWNERS~~ [D3Hunter,ywqzzy]

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-28T08:38:56Z

[LGTM Timeline notifier]

Timeline:

2023-12-28 07:42:26.25448349 +0000 UTC m=+1724437.291710418: ☑️ agreed by ywqzzy.
2023-12-28 08:38:55.116359409 +0000 UTC m=+1727826.153586337: ☑️ agreed by D3Hunter.

okJiang · 2023-12-28T08:43:54Z

/retest

Refactor slotManager to handle task occupation

8928d78

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 22, 2023

okJiang changed the title ~~disttask: Refactor slotManager to handle task occupation~~ disttask: Refactor taskExecutor slotManager to handle task occupation Dec 22, 2023

ti-chi-bot bot requested review from D3Hunter, tangenta and ywqzzy December 22, 2023 09:42

D3Hunter mentioned this pull request Dec 25, 2023

resource control inside dist framework #49008

Open

43 tasks

D3Hunter reviewed Dec 25, 2023

View reviewed changes

fix comment: Refactor slotManager to improve task allocation efficiency

f703d67

D3Hunter reviewed Dec 26, 2023

View reviewed changes

pkg/disttask/framework/taskexecutor/manager.go Outdated Show resolved Hide resolved

fix comment: cancel occupied subtask normally and add ut

202ea21

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 Dec 26, 2023

Update pkg/disttask/framework/taskexecutor/manager.go

eee0420

Co-authored-by: D3Hunter <[email protected]>

ywqzzy reviewed Dec 27, 2023

View reviewed changes

D3Hunter reviewed Dec 27, 2023

View reviewed changes

okJiang added 3 commits December 27, 2023 14:11

fix comment

8cff09a

fix comment: add test case

728629c

merge

078762b

okJiang requested review from D3Hunter and ywqzzy December 27, 2023 08:12

fix comment: optimize unit test

4b140b9

ywqzzy reviewed Dec 28, 2023

View reviewed changes

ywqzzy approved these changes Dec 28, 2023

View reviewed changes

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

okJiang commented Dec 28, 2023

View reviewed changes

pkg/disttask/framework/taskexecutor/manager.go Outdated Show resolved Hide resolved

Update pkg/disttask/framework/taskexecutor/manager.go

1109df9

D3Hunter reviewed Dec 28, 2023

View reviewed changes

pkg/disttask/framework/taskexecutor/manager.go Outdated Show resolved Hide resolved

D3Hunter reviewed Dec 28, 2023

View reviewed changes

pkg/disttask/framework/taskexecutor/manager_test.go Outdated Show resolved Hide resolved

okJiang commented Dec 28, 2023

View reviewed changes

pkg/disttask/framework/taskexecutor/manager.go Outdated Show resolved Hide resolved

Apply suggestions from code review

47a7e05

Co-authored-by: D3Hunter <[email protected]>

D3Hunter approved these changes Dec 28, 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 28, 2023

ti-chi-bot bot merged commit a1d2b50 into pingcap:master Dec 28, 2023
18 checks passed

okJiang deleted the disttask-rc/cancel-subtasks branch December 28, 2023 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disttask: Refactor taskExecutor slotManager to handle task occupation #49713

disttask: Refactor taskExecutor slotManager to handle task occupation #49713

okJiang commented Dec 22, 2023

codecov bot commented Dec 22, 2023 •

edited

Loading

okJiang commented Dec 22, 2023

D3Hunter Dec 25, 2023

D3Hunter Dec 25, 2023

ywqzzy Dec 25, 2023

okJiang Dec 26, 2023

D3Hunter Dec 25, 2023

okJiang Dec 25, 2023 •

edited

Loading

ywqzzy Dec 25, 2023

D3Hunter Dec 25, 2023

D3Hunter Dec 25, 2023

D3Hunter Dec 25, 2023

purelind commented Dec 26, 2023

okJiang commented Dec 26, 2023

okJiang commented Dec 27, 2023

ywqzzy Dec 27, 2023

okJiang Dec 27, 2023 •

edited

Loading

ywqzzy Dec 27, 2023

okJiang Dec 27, 2023 •

edited

Loading

okJiang Dec 28, 2023

D3Hunter Dec 27, 2023

D3Hunter Dec 27, 2023

okJiang Dec 27, 2023

D3Hunter Dec 28, 2023

okJiang Dec 28, 2023 •

edited

Loading

D3Hunter Dec 28, 2023

ywqzzy Dec 28, 2023

okJiang Dec 28, 2023

ywqzzy Dec 28, 2023

ywqzzy Dec 28, 2023

D3Hunter left a comment

ti-chi-bot bot commented Dec 28, 2023

ti-chi-bot bot commented Dec 28, 2023

okJiang commented Dec 28, 2023

	// Pause all running subtasks, don't mark subtasks as canceled.
	// Pause given subtasks temporarily, don't mark subtasks as canceled.

disttask: Refactor taskExecutor slotManager to handle task occupation #49713

disttask: Refactor taskExecutor slotManager to handle task occupation #49713

Conversation

okJiang commented Dec 22, 2023

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

codecov bot commented Dec 22, 2023 • edited Loading

Codecov Report

okJiang commented Dec 22, 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

Choose a reason for hiding this comment

okJiang Dec 25, 2023 • edited Loading

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

purelind commented Dec 26, 2023

okJiang commented Dec 26, 2023

okJiang commented Dec 27, 2023

Choose a reason for hiding this comment

okJiang Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okJiang Dec 27, 2023 • edited Loading

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

okJiang Dec 28, 2023 • edited Loading

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

D3Hunter left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 28, 2023

ti-chi-bot bot commented Dec 28, 2023

[LGTM Timeline notifier]

okJiang commented Dec 28, 2023

codecov bot commented Dec 22, 2023 •

edited

Loading

okJiang Dec 25, 2023 •

edited

Loading

okJiang Dec 27, 2023 •

edited

Loading

okJiang Dec 27, 2023 •

edited

Loading

okJiang Dec 28, 2023 •

edited

Loading