disttask: add load data dispatcher #42592

GMHDBJD · 2023-03-26T18:07:58Z

What problem does this PR solve?

Issue Number: close #42591

Problem Summary:

What is changed and how it works?

split files into chunks

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

ti-chi-bot · 2023-03-26T18:08:00Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

D3Hunter
lance6716

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

hawkingrei · 2023-03-27T02:40:07Z

disttask/loaddata/BUILD.bazel

+)
+
+go_test(
+    name = "loaddata_test",


Please add

timeout = "short", flaky = True,

GMHDBJD · 2023-03-27T09:18:46Z

/retest

disttask/loaddata/dispatcher.go

disttask/framework/dispatcher/dispatcher.go

lance6716 · 2023-03-28T03:15:46Z

disttask/framework/dispatcher/dispatcher_mock.go

in https://github.com/stretchr/testify#mock-package

You can use the mockery tool to autogenerate the mock code against an interface as well, making using mocks much quicker.

Can we add a makefile entry for it?

I can add in later pr with https://github.com/pingcap/tidb/blob/master/disttask/framework/scheduler/interface_mock.go

lance6716 · 2023-03-28T03:17:34Z

disttask/framework/dispatcher/register.go

 	"github.com/pingcap/tidb/disttask/framework/proto"
 	"github.com/pingcap/tidb/util/syncutil"
 )

 // TaskFlowHandle is used to control the process operations for each global task.
 type TaskFlowHandle interface {
-	ProcessNormalFlow(d Dispatch, gTask *proto.Task) (metas [][]byte, err error)
-	ProcessErrFlow(d Dispatch, gTask *proto.Task, receive string) (meta []byte, err error)
+	ProcessNormalFlow(ctx context.Context, h Handle, gTask *proto.Task) (metas [][]byte, err error)


the comment says this is for each task, so why we need to specify gTask as argument?

TaskFlowHandle gets the gTask from dispatcher, and then split the gTask to subtask

Seems different types of task has different TaskFlowHandle implementation, can we use gTask to create a new struct as the receiver of these methods? In other words, ProcessNormalFlow is a method of task object, to avoid use gTask as parameter.

PTAL @zimulala , maybe let RegisterTaskFlowHandle register a factory function

lance6716 · 2023-03-28T10:02:27Z

disttask/framework/dispatcher/register.go

 	"github.com/pingcap/tidb/disttask/framework/proto"
 	"github.com/pingcap/tidb/util/syncutil"
 )

 // TaskFlowHandle is used to control the process operations for each global task.
 type TaskFlowHandle interface {
-	ProcessNormalFlow(d Dispatch, gTask *proto.Task) (metas [][]byte, err error)
-	ProcessErrFlow(d Dispatch, gTask *proto.Task, receive string) (meta []byte, err error)
+	ProcessNormalFlow(ctx context.Context, h Handle, gTask *proto.Task) (metas [][]byte, err error)


Seems different types of task has different TaskFlowHandle implementation, can we use gTask to create a new struct as the receiver of these methods? In other words, ProcessNormalFlow is a method of task object, to avoid use gTask as parameter.

disttask/loaddata/dispatcher.go

lance6716 · 2023-03-28T10:10:18Z

disttask/loaddata/dispatcher.go

+
+	switch gTask.Step {
+	case Import:
+		gTask.State = proto.TaskStateSucceed


What's the difference of Step and State? Please add comments

https://github.com/pingcap/tidb/blob/master/disttask/framework/proto/task.go#L21-L51

Will each step has the full lifetime of state transition? Or step is changed only in TaskStateRunning and only the final step will finish the state?

Please add comment

lance6716 · 2023-03-28T10:13:39Z

disttask/loaddata/dispatcher.go

+		}
+		metaBytes = append(metaBytes, bs)
+	}
+	gTask.Step = Import


please add comment to the interface to mention that gTask will be modified inside this method

@zimulala Not sure if you can fix it in your PR conveniently

I'll comment on that

disttask/loaddata/dispatcher.go

lance6716 · 2023-03-28T10:21:15Z

disttask/loaddata/dispatcher.go

+}
+
+func init() {
+	dispatcher.RegisterTaskFlowHandle(proto.LoadData, &FlowHandle{})


no need to use lock inside RegisterTaskFlowHandle?

https://go.dev/ref/spec

Package initialization—variable initialization and the invocation of init functions—happens in a single goroutine, sequentially, one package at a time.

But we're not sure that all the calls are inside the init function

lance6716 · 2023-03-29T03:52:50Z

disttask/loaddata/dispatcher.go

+
+	switch gTask.Step {
+	case Import:
+		gTask.State = proto.TaskStateSucceed


Will each step has the full lifetime of state transition? Or step is changed only in TaskStateRunning and only the final step will finish the state?

Please add comment

lance6716 · 2023-03-29T04:01:35Z

disttask/framework/dispatcher/register.go

 	"github.com/pingcap/tidb/disttask/framework/proto"
 	"github.com/pingcap/tidb/util/syncutil"
 )

 // TaskFlowHandle is used to control the process operations for each global task.
 type TaskFlowHandle interface {
-	ProcessNormalFlow(d Dispatch, gTask *proto.Task) (metas [][]byte, err error)
-	ProcessErrFlow(d Dispatch, gTask *proto.Task, receive string) (meta []byte, err error)
+	ProcessNormalFlow(ctx context.Context, h Handle, gTask *proto.Task) (metas [][]byte, err error)


PTAL @zimulala , maybe let RegisterTaskFlowHandle register a factory function

lance6716 · 2023-03-29T04:02:33Z

disttask/loaddata/dispatcher.go

+		}
+		metaBytes = append(metaBytes, bs)
+	}
+	gTask.Step = Import


@zimulala Not sure if you can fix it in your PR conveniently

lance6716 · 2023-03-29T04:02:44Z

disttask/loaddata/dispatcher.go

+}
+
+func init() {
+	dispatcher.RegisterTaskFlowHandle(proto.LoadData, &FlowHandle{})


D3Hunter · 2023-03-29T04:14:32Z

disttask/framework/dispatcher/dispatcher.go

@@ -50,6 +50,12 @@ type Dispatch interface {
 	Stop()
 }

+// Handle provides the interface for operations needed by task flow handles.
+type Handle interface {


Suggested change

type Handle interface {

type TaskHandle interface {

D3Hunter · 2023-03-29T04:14:49Z

disttask/framework/dispatcher/dispatcher.go

+// Handle provides the interface for operations needed by task flow handles.
+type Handle interface {
+	// GetTaskAllInstances gets handles the task's all available instances.
+	GetTaskAllInstances(ctx context.Context, gTaskID int64) ([]string, error)


GetAllSchedulerIDs?

D3Hunter · 2023-03-29T04:31:34Z

disttask/framework/dispatcher/dispatcher_mock.go

generate it with mockgen? and put it into Makefile

ok, in next pr #42592 (comment)

hawkingrei · 2023-03-29T05:29:51Z

disttask/framework/dispatcher/BUILD.bazel

@@ -16,6 +17,7 @@ go_library(
        "//util/logutil",
        "//util/syncutil",
        "@com_github_pingcap_errors//:errors",
+        "@com_github_stretchr_testify//mock",


I suggest we can add race = "on", to go_test for finding the data race. You can decide whether to open it according to the situation.

GMHDBJD · 2023-03-29T05:47:33Z

/merge

ti-chi-bot · 2023-03-29T05:47:37Z

This pull request has been accepted and is ready to merge.

Commit hash: 4adce49

GMHDBJD added 2 commits March 27, 2023 01:57

disttask: add dispatcher for load data

f975ad5

update

5ad5939

ti-chi-bot added do-not-merge/invalid-title release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 26, 2023

GMHDBJD changed the title ~~Add load data dispatcher~~ disttask: add load data dispatcher Mar 26, 2023

ti-chi-bot removed the do-not-merge/invalid-title label Mar 26, 2023

hawkingrei reviewed Mar 27, 2023

View reviewed changes

disttask/loaddata/BUILD.bazel

)

go_test(

name = "loaddata_test",

Copy link

Member

hawkingrei Mar 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add

timeout = "short", flaky = True,

GMHDBJD added 2 commits March 27, 2023 13:26

update bazel

d007732

add ut

34665c4

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 27, 2023

GMHDBJD added 2 commits March 27, 2023 15:48

fix ut

816ec8c

fix ut

1deabfa

hawkingrei reviewed Mar 27, 2023

View reviewed changes

disttask/loaddata/dispatcher.go Show resolved Hide resolved

lance6716 reviewed Mar 28, 2023

View reviewed changes

address comment

b7f372a

lance6716 approved these changes Mar 29, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 29, 2023

D3Hunter reviewed Mar 29, 2023

View reviewed changes

address comment

4adce49

hawkingrei reviewed Mar 29, 2023

View reviewed changes

D3Hunter approved these changes Mar 29, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 29, 2023

hawkingrei approved these changes Mar 29, 2023

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 29, 2023

ti-chi-bot merged commit af62a5c into pingcap:master Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disttask: add load data dispatcher #42592

disttask: add load data dispatcher #42592

GMHDBJD commented Mar 26, 2023

ti-chi-bot commented Mar 26, 2023 •

edited

Loading

hawkingrei Mar 27, 2023

GMHDBJD commented Mar 27, 2023

lance6716 Mar 28, 2023

GMHDBJD Mar 28, 2023

lance6716 Mar 28, 2023

GMHDBJD Mar 28, 2023

lance6716 Mar 28, 2023

lance6716 Mar 29, 2023

lance6716 Mar 28, 2023

lance6716 Mar 28, 2023

GMHDBJD Mar 29, 2023

lance6716 Mar 29, 2023

lance6716 Mar 28, 2023 •

edited

Loading

lance6716 Mar 29, 2023

zimulala Mar 29, 2023

lance6716 Mar 28, 2023

lance6716 Mar 29, 2023

zimulala Mar 29, 2023

lance6716 Mar 29, 2023

lance6716 Mar 29, 2023

lance6716 Mar 29, 2023

lance6716 Mar 29, 2023

D3Hunter Mar 29, 2023

D3Hunter Mar 29, 2023

GMHDBJD Mar 29, 2023

D3Hunter Mar 29, 2023

GMHDBJD Mar 29, 2023

hawkingrei Mar 29, 2023 •

edited

Loading

GMHDBJD commented Mar 29, 2023

ti-chi-bot commented Mar 29, 2023

disttask: add load data dispatcher #42592

disttask: add load data dispatcher #42592

Conversation

GMHDBJD commented Mar 26, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 26, 2023 • edited Loading

Choose a reason for hiding this comment

GMHDBJD commented Mar 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 Mar 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

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

hawkingrei Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

GMHDBJD commented Mar 29, 2023

ti-chi-bot commented Mar 29, 2023

ti-chi-bot commented Mar 26, 2023 •

edited

Loading

lance6716 Mar 28, 2023 •

edited

Loading

hawkingrei Mar 29, 2023 •

edited

Loading