distribute framework: restrict the task state transform rule #45932

GMHDBJD · 2023-08-09T07:02:14Z

What problem does this PR solve?

Issue Number: ref #41495

Problem Summary:

What is changed and how it works?

restrict the task state transform rule

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

tiprow · 2023-08-09T07:02:43Z

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

codecov · 2023-08-09T07:18:57Z

Codecov Report

Merging #45932 (5b7c9fe) into master (772275c) will decrease coverage by 2.7972%.
Report is 76 commits behind head on master.
The diff coverage is 87.3015%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #45932        +/-   ##
================================================
- Coverage   74.7799%   71.9828%   -2.7972%     
================================================
  Files          1293       1308        +15     
  Lines        397910     412340     +14430     
================================================
- Hits         297557     296814       -743     
- Misses        80985      96717     +15732     
+ Partials      19368      18809       -559

Flag	Coverage Δ
integration	`27.1171% <0.0000%> (-19.3716%)`	⬇️
unit	`73.3625% <94.8275%> (+0.0205%)`	⬆️

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

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`85.0656% <ø> (+0.0054%)`	⬆️
br	`47.2296% <ø> (-17.4618%)`	⬇️

ywqzzy · 2023-08-09T08:01:42Z

/cc @ywqzzy

GMHDBJD · 2023-08-09T09:40:50Z

/retest

tiprow · 2023-08-09T09:41:33Z

@GMHDBJD: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

GMHDBJD · 2023-08-10T07:33:34Z

/retest

tiprow · 2023-08-10T07:35:13Z

@GMHDBJD: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

GMHDBJD · 2023-08-10T09:34:11Z

/ok-to-test

ywqzzy · 2023-08-11T02:35:05Z

disttask/framework/dispatcher/dispatcher.go

+	if !VerifyTaskStateTransform(prevState, taskState) {
+		return errors.Errorf("invalid task state transform, from %s to %s", prevState, taskState)
+	}
+	failpoint.Inject("syncInUpdateTask", func(_ failpoint.Value) {


Seems not used.

ywqzzy · 2023-08-11T02:37:18Z

disttask/framework/proto/task.go

@@ -41,6 +41,7 @@ const (
 	TaskStateCanceled      = "canceled"
 	TaskStatePausing       = "pausing"
 	TaskStatePaused        = "paused"
+	TaskStateResuming      = "resuming"


Need more discussion to introduce a new state.
For now, LGTM.

Further more, we can distinguish subtask state and task state later.

ti-chi-bot · 2023-08-11T02:39:29Z

@ywqzzy: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

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-08-17T08:47:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tangenta, wjhuang2016, 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~~ [tangenta,wjhuang2016]

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-08-17T08:47:30Z

[LGTM Timeline notifier]

Timeline:

2023-08-17 08:43:47.361154913 +0000 UTC m=+793391.910170900: ☑️ agreed by wjhuang2016.
2023-08-17 08:47:29.410069969 +0000 UTC m=+793613.959085956: ☑️ agreed by tangenta.

GMHDBJD · 2023-08-17T09:16:36Z

/merge

ti-chi-bot · 2023-08-17T09:16:38Z

@GMHDBJD: We have migrated to builtin LGTM and approve plugins for reviewing.

Please use /approve when you want approve this pull request.

The changes announcement: LGTM plugin changes

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.

GMHDBJD · 2023-08-18T05:49:35Z

/retest

D3Hunter · 2023-08-18T08:38:50Z

disttask/framework/storage/task_table.go

-		_, err := ExecSQL(stm.ctx, se, "update mysql.tidb_global_task set state = %?, dispatcher_id = %?, step = %?, state_update_time = %?, concurrency = %?, meta = %?, error = %? where id = %?",
-			gTask.State, gTask.DispatcherID, gTask.Step, gTask.StateUpdateTime.UTC().String(), gTask.Concurrency, gTask.Meta, serializeErr(gTask.Error), gTask.ID)
+		_, err := ExecSQL(stm.ctx, se, "update mysql.tidb_global_task set state = %?, dispatcher_id = %?, step = %?, state_update_time = %?, concurrency = %?, meta = %?, error = %? where id = %? and state = %?",
+			gTask.State, gTask.DispatcherID, gTask.Step, gTask.StateUpdateTime.UTC().String(), gTask.Concurrency, gTask.Meta, serializeErr(gTask.Error), gTask.ID, prevState)


we need check how many lines're affected, if 0, we shouldn't update subtasks

we need check how many lines're affected, if 0, we shouldn't update subtasks

I will fix it.

GMHDBJD added 3 commits August 9, 2023 13:40

add verifyTaskStateTransform

ab5ce57

update

80add38

add ut

f21741f

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 9, 2023

add bazel

f0df305

GMHDBJD added 2 commits August 9, 2023 15:23

fix lint

76864c9

update

52cfc93

ti-chi-bot bot requested a review from ywqzzy August 9, 2023 08:01

update

6cf9d19

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 9, 2023

fix ut

be5b72a

GMHDBJD added 2 commits August 9, 2023 18:06

update

2e7ccc1

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

346df35

update

aef26e9

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

ywqzzy approved these changes Aug 11, 2023

View reviewed changes

address comment

5b7c9fe

wjhuang2016 approved these changes Aug 17, 2023

View reviewed changes

ti-chi-bot bot added the approved label Aug 17, 2023

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

tangenta approved these changes Aug 17, 2023

View reviewed changes

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

ti-chi-bot bot merged commit 883e88a into pingcap:master Aug 18, 2023
4 of 9 checks passed

D3Hunter reviewed Aug 18, 2023

View reviewed changes

ywqzzy mentioned this pull request Aug 23, 2023

Enhance distributed task execution framework #46258

Closed

76 tasks

ywqzzy mentioned this pull request Nov 15, 2023

dist framework: cancelled task status is overriden to running #44443

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

distribute framework: restrict the task state transform rule #45932

distribute framework: restrict the task state transform rule #45932

GMHDBJD commented Aug 9, 2023

tiprow bot commented Aug 9, 2023

codecov bot commented Aug 9, 2023 •

edited

Loading

ywqzzy commented Aug 9, 2023

GMHDBJD commented Aug 9, 2023

tiprow bot commented Aug 9, 2023

GMHDBJD commented Aug 10, 2023

tiprow bot commented Aug 10, 2023

GMHDBJD commented Aug 10, 2023

ywqzzy Aug 11, 2023

ywqzzy Aug 11, 2023

ywqzzy Aug 11, 2023

ti-chi-bot bot commented Aug 11, 2023

ti-chi-bot bot commented Aug 17, 2023

ti-chi-bot bot commented Aug 17, 2023

GMHDBJD commented Aug 17, 2023

ti-chi-bot bot commented Aug 17, 2023

GMHDBJD commented Aug 18, 2023

D3Hunter Aug 18, 2023

ywqzzy Aug 18, 2023

distribute framework: restrict the task state transform rule #45932

distribute framework: restrict the task state transform rule #45932

Conversation

GMHDBJD commented Aug 9, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

tiprow bot commented Aug 9, 2023

codecov bot commented Aug 9, 2023 • edited Loading

Codecov Report

ywqzzy commented Aug 9, 2023

GMHDBJD commented Aug 9, 2023

tiprow bot commented Aug 9, 2023

GMHDBJD commented Aug 10, 2023

tiprow bot commented Aug 10, 2023

GMHDBJD commented Aug 10, 2023

ywqzzy Aug 11, 2023

Choose a reason for hiding this comment

ywqzzy Aug 11, 2023

Choose a reason for hiding this comment

ywqzzy Aug 11, 2023

Choose a reason for hiding this comment

ti-chi-bot bot commented Aug 11, 2023

ti-chi-bot bot commented Aug 17, 2023

ti-chi-bot bot commented Aug 17, 2023

[LGTM Timeline notifier]

GMHDBJD commented Aug 17, 2023

ti-chi-bot bot commented Aug 17, 2023

GMHDBJD commented Aug 18, 2023

D3Hunter Aug 18, 2023

Choose a reason for hiding this comment

ywqzzy Aug 18, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2023 •

edited

Loading