disttask: add finish flow for distribute framework #43676

GMHDBJD · 2023-05-10T04:34:30Z

What problem does this PR solve?

Issue Number: close #43675

Problem Summary:

What is changed and how it works?

scheduler update subtask results by OnSubtaskFinished
dispatcher fetch subtask results and then pass it to business handle by processFinishFlow.

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-05-10T04:34:33Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

D3Hunter
zimulala

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.

D3Hunter

15/19 done

D3Hunter · 2023-05-10T06:38:13Z

disttask/framework/dispatcher/register.go

+	// ProcessNormalFlow processes the normal flow.
+	// It receives the previous subtask metas to do some post-processing.
+	// returns the new subtask metas and whether the error is retryable.
+	ProcessNormalFlow(ctx context.Context, h TaskHandle, gTask *proto.Task, prevSubtaskMetas [][]byte) (subtaskMetas [][]byte, retryable bool, err error)


maybe just add a new api to this TaskFlowHandle to check retriable

I prefer do retry in business logic, so this retryable variable is just a temporary solution.

Whether being able to retry should be a attribute of each step, and the dispatcher or scheduler decides the follow-up action according to that attribute.

The user story could be:

The framework provide the capability to set the attribute

Each task / job / business (or whatever you called it) should have such kind of attributes or definitions which define how the framework trigger the steps and control the flow

The task that need to be paused on the failure, define the value of the attribute

The framework takes actions according to the definition of attributes

API may not be a good idea because we may need another system or mechanism to call such API accordingly which breaks the cohesion.

disttask/framework/dispatcher/dispatcher.go

disttask/framework/framework_test.go

D3Hunter · 2023-05-10T07:21:34Z

disttask/framework/scheduler/interface_mock.go

 	args := m.Called(ctx, subtask)
-	return args.Error(0)
+	if args.Error(1) != nil {


is this file manually mocked?

Yes, I will change to generated in another pr

D3Hunter

rest lgmt

D3Hunter · 2023-05-10T07:32:42Z

session/bootstrap.go

+	if ver >= version145 {
+		return
+	}
+	doReentrantDDL(s, "ALTER TABLE mysql.tidb_background_subtask ADD COLUMN `step` INT", infoschema.ErrColumnExists)


this will add step the last column of the table, but in row2SubTask we use Step: r.GetInt64(1)

D3Hunter

rest lgtm

disttask/framework/dispatcher/dispatcher_test.go

zimulala · 2023-05-11T11:46:36Z

disttask/framework/dispatcher/dispatcher.go

+	for _, subtask := range prevSubtasks {
+		prevSubtaskMetas = append(prevSubtaskMetas, subtask.Meta)
+	}
+	metas, retryable, err := handle.ProcessNormalFlow(d.ctx, d, gTask, prevSubtaskMetas)
 	if err != nil {
 		logutil.BgLogger().Warn("gen dist-plan failed", zap.Error(err))


Add retryable to this log?

zimulala · 2023-05-12T06:51:55Z

disttask/framework/dispatcher/dispatcher.go

+	for _, subtask := range prevSubtasks {
+		prevSubtaskMetas = append(prevSubtaskMetas, subtask.Meta)
+	}
+	metas, retryable, err := handle.ProcessNormalFlow(d.ctx, d, gTask, prevSubtaskMetas)


What does prevSubtasks do? I don't think it's in use at the moment. In addition, this quantity may be very large, is there any other way to deal with it? For example, put it in gTask to reshard the task.

Rest LGTM

prevSubtaskMetas contains the result of previous step

This is not a good idea.

If we need to record all the previous sub task, we may need to record the whole sub-task context because it could fail on any sub-task. The sub-tasks should not depends on each other, they should all depends on another thing, such as context or meta.

Refer to 'Dependence Inversion Principle'.

zimulala · 2023-05-15T07:41:35Z

disttask/framework/dispatcher/dispatcher.go

+			if stepIsFinished && len(errStr) == 0 && gTask.State == proto.TaskStateRunning {
+				logutil.BgLogger().Info("detect task, this task finished a step",
+					zap.Int64("taskID", gTask.ID), zap.String("state", gTask.State), zap.Int64("step", gTask.Step))
+				if err := d.processFinishFlow(gTask); err != nil {


I think we can put processFinishFlow directly, and only import step2 or other special step needs to do this.

This reverts commit 74f7e26.

This reverts commit 9c2624e.

GMHDBJD · 2023-05-15T08:48:27Z

/retest

GMHDBJD · 2023-05-15T08:52:36Z

/retest

GMHDBJD · 2023-05-15T09:01:49Z

/test build

zimulala

LGTM

GMHDBJD · 2023-05-15T09:26:48Z

/merge

ti-chi-bot · 2023-05-15T09:26:52Z

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

Commit hash: 76f8c68

GMHDBJD · 2023-05-15T13:25:22Z

/merge

ti-chi-bot · 2023-05-15T13:25:28Z

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

Commit hash: b85b9a2

disttask: add finish flow for distribute framwork

fefc43a

GMHDBJD requested a review from a team as a code owner May 10, 2023 04:34

ti-chi-bot bot added release-note-none size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 10, 2023

fix ut

621fb15

D3Hunter reviewed May 10, 2023

View reviewed changes

address comment

5090506

D3Hunter approved these changes May 11, 2023

View reviewed changes

disttask/framework/dispatcher/dispatcher_test.go Outdated Show resolved Hide resolved

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

GMHDBJD added 2 commits May 11, 2023 14:47

update

bda851c

Merge remote-tracking branch 'upstream/master' into addFinishFlow

ac1a517

zimulala reviewed May 11, 2023

View reviewed changes

address comment

7409d80

zimulala reviewed May 12, 2023

View reviewed changes

GMHDBJD changed the title ~~disttask: add finish flow for distribute framework~~ disttask: pass previous subtask result to next step in dispatcher May 15, 2023

GMHDBJD added 5 commits May 15, 2023 11:43

Merge remote-tracking branch 'upstream/master' into addFinishFlow

d111466

address comment

8332da0

add process finish flow

9c2624e

fix

74f7e26

Merge remote-tracking branch 'upstream/master' into addFinishFlow

899437d

GMHDBJD changed the title ~~disttask: pass previous subtask result to next step in dispatcher~~ disttask: add finish flow for distribute framework May 15, 2023

zimulala reviewed May 15, 2023

View reviewed changes

GMHDBJD added 4 commits May 15, 2023 15:53

Revert "fix"

6cfe8c6

This reverts commit 74f7e26.

Revert "add process finish flow"

7b69d73

This reverts commit 9c2624e.

move func to dispatcher handle

e67e230

update

fc6f1e6

zimulala approved these changes May 15, 2023

View reviewed changes

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

update

76f8c68

qw4990 approved these changes May 15, 2023

View reviewed changes

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

add log

b85b9a2

ti-chi-bot bot removed the status/can-merge Indicates a PR has been approved by a committer. label May 15, 2023

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

ti-chi-bot bot merged commit a60cf97 into pingcap:master May 15, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disttask: add finish flow for distribute framework #43676

disttask: add finish flow for distribute framework #43676

GMHDBJD commented May 10, 2023 •

edited

ti-chi-bot bot commented May 10, 2023 •

edited

D3Hunter left a comment

D3Hunter May 10, 2023

GMHDBJD May 11, 2023

dhysum May 15, 2023 •

edited

D3Hunter May 10, 2023

GMHDBJD May 11, 2023 •

edited

D3Hunter left a comment

D3Hunter May 10, 2023

D3Hunter left a comment

zimulala May 11, 2023

zimulala May 12, 2023

GMHDBJD May 15, 2023

dhysum May 15, 2023 •

edited

zimulala May 15, 2023

GMHDBJD commented May 15, 2023

GMHDBJD commented May 15, 2023

GMHDBJD commented May 15, 2023

zimulala left a comment

GMHDBJD commented May 15, 2023

ti-chi-bot bot commented May 15, 2023

GMHDBJD commented May 15, 2023

ti-chi-bot bot commented May 15, 2023

disttask: add finish flow for distribute framework #43676

disttask: add finish flow for distribute framework #43676

Conversation

GMHDBJD commented May 10, 2023 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented May 10, 2023 • edited

D3Hunter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhysum May 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD May 11, 2023 • edited

Choose a reason for hiding this comment

D3Hunter left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhysum May 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD commented May 15, 2023

GMHDBJD commented May 15, 2023

GMHDBJD commented May 15, 2023

zimulala left a comment

Choose a reason for hiding this comment

GMHDBJD commented May 15, 2023

ti-chi-bot bot commented May 15, 2023

GMHDBJD commented May 15, 2023

ti-chi-bot bot commented May 15, 2023

GMHDBJD commented May 10, 2023 •

edited

ti-chi-bot bot commented May 10, 2023 •

edited

dhysum May 15, 2023 •

edited

GMHDBJD May 11, 2023 •

edited

dhysum May 15, 2023 •

edited