disttask: task executor slot manager #49136

okJiang · 2023-12-04T08:15:54Z

What problem does this PR solve?

Issue Number: close #49135

Problem Summary:

In distributed framework, we introduced slot manager to control resource.

What changed and how does it work?

in scheduler(task executor)

check the available slot before open scheduler
count the slot by slot manager

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

…k-rc/scheduler-slot-manager

ti-chi-bot · 2023-12-04T08:15:57Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2023-12-04T08:16:03Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

…k-rc/scheduler-slot-manager

okJiang · 2023-12-05T06:50:49Z

/ok-to-test

ywqzzy · 2023-12-05T06:55:56Z

pkg/disttask/framework/taskexecutor/manager_test.go

@@ -179,7 +185,7 @@ func TestManager(t *testing.T) {
 	mockTaskTable := mock.NewMockTaskTable(ctrl)
 	mockInternalExecutor := mock.NewMockTaskExecutor(ctrl)
 	mockPool := mock.NewMockPool(ctrl)
-	b := NewManagerBuilder()


add manager test with slot_manager

ywqzzy · 2023-12-05T06:57:33Z

pkg/ddl/constant.go

@@ -79,6 +80,7 @@ const (
 		exec_id varchar(256),
 		exec_expired timestamp,
 		state varchar(64) not null,
+		priority int,


it is useless, I will remove it

ywqzzy · 2023-12-05T07:01:36Z

pkg/disttask/framework/taskexecutor/manager.go

+	priorityQueue := make(proto.TaskPriorityQueue, 0)
+	heap.Init(&priorityQueue)
 	for _, task := range tasks {
+		heap.Push(&priorityQueue, proto.WrapPriorityQueue(task))
+	}


Not considering handling tasks

has been filtered

has been filtered

But we will take running tasks into consideration.

has been filtered

But we should take running tasks into consideration.

Do you mean after TiDB restarts?

ywqzzy · 2023-12-05T07:03:02Z

pkg/disttask/framework/taskexecutor/slot_manager.go

+
+type slotInfo struct {
+	taskID    int
+	priority  int


Unused for now.

it is reserved, I can add a comment for it.

ywqzzy · 2023-12-05T07:03:42Z

pkg/disttask/framework/taskexecutor/manager.go

@@ -45,6 +47,7 @@ var (
 // ManagerBuilder is used to build a Manager.
 type ManagerBuilder struct {
 	newPool func(name string, size int32, component util.Component, options ...spool.Option) (Pool, error)
+	// slotAlloctor alloctor.Alloctor


ywqzzy · 2023-12-05T07:04:35Z

pkg/disttask/framework/taskexecutor/manager_test.go

+func NewTestManagerBuilder(ctrl *gomock.Controller) *ManagerBuilder {
 	b := NewManagerBuilder()
+	return b
+}


Why add this function?

ywqzzy · 2023-12-05T07:05:59Z

pkg/disttask/framework/mock/alloctor_mock.go

+type MockAlloctor struct {
+	ctrl     *gomock.Controller
+	recorder *MockAlloctorMockRecorder
+}


Seems not used for now

ywqzzy · 2023-12-05T07:14:45Z

pkg/resourcemanager/BUILD.bazel

@@ -3,7 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 go_library(
    name = "resourcemanager",
    srcs = [
-        "rm.go",
+        "resource_manager.go",


revert the change?

resource_manager.go is more meaningful

resource_manager.go is more meaningful

Not the code of disttask

ywqzzy · 2023-12-14T04:21:03Z

pkg/disttask/framework/taskexecutor/slot_manager.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package taskexecutor


slot_manager.go. ----> slots.go

ywqzzy · 2023-12-14T04:21:28Z

pkg/disttask/framework/taskexecutor/slot_manager_test.go

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+


slot_manager_test.go ----> slots_test.go

This commit refactors the slot management in the task executor module. It introduces a new `slotManager` struct that handles the reservation and release of slots for executing tasks. The `slotManager` keeps track of the available slots and the tasks currently occupying the slots. The changes include renaming some files and functions for better clarity. The `slot_manager.go` file is renamed to `slot.go`, and the `slot_manager_test.go` file is renamed to `slot_test.go`. The `slotManager` struct now has a `reserve` method to reserve slots for a task and an `unReserve` method to release the slots when the task is completed. These changes improve the readability and maintainability of the code, making it easier to understand and modify the slot management logic in the task executor.

pkg/disttask/framework/taskexecutor/slot.go

ywqzzy

Rest LGTM

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

okJiang · 2023-12-18T07:20:54Z

/retest

tangenta · 2023-12-18T08:23:11Z

pkg/disttask/framework/taskexecutor/slot.go

+
+// canReserve is used to check whether the instance has enough slots to run the task.
+func (sm *slotManager) canReserve(task *proto.Task) bool {
+	return sm.available >= task.Concurrency


pkg/disttask/framework/taskexecutor/manager.go

D3Hunter · 2023-12-18T11:40:20Z

pkg/disttask/framework/taskexecutor/slot_test.go

+const (
+	managerID = "test"
+	taskID    = int64(1)
+	taskID2   = int64(2)
+)


those simple var can just duplicate in test where it's used

D3Hunter · 2023-12-18T11:49:53Z

pkg/disttask/framework/taskexecutor/manager_test.go

+	defer func() {
+		failpoint.Disable("github.com/pingcap/tidb/pkg/disttask/framework/taskexecutor/taskTick")
+	}()
+	go m.fetchAndHandleRunnableTasksLoop()


can call onRunnableTasks directly to simplify mock, and we will not need failpoint anymore

If not use failpoint, how can we check the status of slotManager?🤔

you can call onRunnableTasks with 1 task and check in this case

D3Hunter · 2023-12-18T11:54:38Z

pkg/disttask/framework/taskexecutor/manager.go

+
+		failpoint.Inject("taskTick", func() {
+			<-onRunnableTaskTick
+		})


not needed, just let executor ext block on Run using mock

D3Hunter · 2023-12-18T11:59:17Z

pkg/disttask/framework/taskexecutor/slot.go

+type slotInfo struct {
+	taskID int
+	// priority will be used in future
+	priority  int


better store task itself, priority itself cannot determine task order. ok to do it later

pkg/disttask/framework/taskexecutor/slot.go

…k-rc/scheduler-slot-manager

wuhuizuo · 2023-12-19T03:31:19Z

/remove-label needs-rebase

ti-chi-bot · 2023-12-19T03:31:22Z

@wuhuizuo: The label(s) needs-rebase cannot be applied. These labels are supported: fuzz/sqlancer, challenge-program, compatibility-breaker, first-time-contributor, contribution, good first issue, correctness, duplicate, proposal, security, ok-to-test, needs-ok-to-test, needs-more-info, needs-cherry-pick-release-5.4, needs-cherry-pick-release-6.1, needs-cherry-pick-release-6.5, needs-cherry-pick-release-7.1, needs-cherry-pick-release-7.5, affects-5.4, affects-6.1, affects-6.5, affects-7.1, affects-7.5, may-affects-5.4, may-affects-6.1, may-affects-6.5, may-affects-7.1, may-affects-7.5.

In response to this:

/remove-label needs-rebase

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 ti-community-infra/tichi repository.

D3Hunter · 2023-12-20T06:07:24Z

pkg/disttask/framework/taskexecutor/manager.go

 			continue
 		}
 		m.addHandlingTask(task.ID)
+		m.slotManager.alloc(task)


if it's moved out, we have to free on error

tiprow · 2023-12-20T06:26:14Z

@okJiang: 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
tiprow_fast_test	`06422f4`	link	true	`/test tiprow_fast_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.

D3Hunter · 2023-12-20T06:47:22Z

/lgtm

ti-chi-bot · 2023-12-20T06:47:30Z

[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:

~~OWNERS~~ [D3Hunter]
~~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-20T06:47:33Z

[LGTM Timeline notifier]

Timeline:

2023-12-18 02:08:04.107448508 +0000 UTC m=+840375.144675437: ☑️ agreed by ywqzzy.
2023-12-20 06:47:32.680304098 +0000 UTC m=+1029943.717531027: ☑️ agreed by D3Hunter.

okJiang added 6 commits November 30, 2023 17:24

rename

6295f58

save work

eae2e5e

Merge branch 'master' of https://github.com/pingcap/tidb into disttas…

45e4b72

…k-rc/scheduler-slot-manager

Merge branch 'master' of https://github.com/pingcap/tidb into disttas…

26e6254

…k-rc/scheduler-slot-manager

rename GetSubtasksByStepAndStates

66c50ed

implement priority quere for task and slotManager

bfc0084

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-triage-completed labels Dec 4, 2023

ti-chi-bot bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 4, 2023

okJiang added 4 commits December 4, 2023 17:17

add ut

6df88d1

fix ut

908b65c

Merge branch 'master' of https://github.com/pingcap/tidb into disttas…

59db269

…k-rc/scheduler-slot-manager

fix bazel

ff93eab

okJiang marked this pull request as ready for review December 5, 2023 06:50

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

okJiang changed the title ~~[wip]dist/task: scheduler slot manager~~ dist/task: scheduler slot manager Dec 5, 2023

ti-chi-bot bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-triage-completed labels Dec 5, 2023

okJiang changed the title ~~dist/task: scheduler slot manager~~ disttask: scheduler slot manager Dec 5, 2023

ywqzzy reviewed Dec 5, 2023

View reviewed changes

add comment, fix bazel

b69f291

ywqzzy reviewed Dec 5, 2023

View reviewed changes

fix comment

f54e5d7

ywqzzy reviewed Dec 5, 2023

View reviewed changes

ywqzzy reviewed Dec 14, 2023

View reviewed changes

okJiang changed the title ~~disttask: scheduler slot manager~~ disttask: task executor slot manager Dec 15, 2023

ywqzzy reviewed Dec 18, 2023

View reviewed changes

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

ywqzzy approved these changes Dec 18, 2023

View reviewed changes

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

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

c442754

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

tangenta reviewed Dec 18, 2023

View reviewed changes

fix comment

3781105

D3Hunter reviewed Dec 18, 2023

View reviewed changes

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

okJiang added 2 commits December 19, 2023 11:18

fix comment

6392faf

Merge branch 'master' of https://github.com/pingcap/tidb into disttas…

7d9f45e

…k-rc/scheduler-slot-manager

wuhuizuo removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 19, 2023

okJiang added 3 commits December 20, 2023 12:52

fix comment: refactor ut

2003d2a

fix comment

a34651f

fix comment

06422f4

D3Hunter reviewed Dec 20, 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 Dec 20, 2023

ti-chi-bot bot merged commit 27aed31 into pingcap:master Dec 20, 2023
17 of 18 checks passed

okJiang deleted the disttask-rc/scheduler-slot-manager branch December 20, 2023 06:54

disttask: task executor slot manager #49136

disttask: task executor slot manager #49136

Conversation

okJiang commented Dec 4, 2023 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Dec 4, 2023

tiprow bot commented Dec 4, 2023

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

okJiang Dec 5, 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

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

ywqzzy left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wuhuizuo commented Dec 19, 2023

ti-chi-bot bot commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiprow bot commented Dec 20, 2023

D3Hunter commented Dec 20, 2023

ti-chi-bot bot commented Dec 20, 2023

ti-chi-bot bot commented Dec 20, 2023

[LGTM Timeline notifier]

okJiang commented Dec 4, 2023 •

edited

Loading

okJiang Dec 5, 2023 •

edited

Loading