disttask: fix cancel from grpc mark subtasks as failed #48339

ywqzzy · 2023-11-07T03:00:32Z

What problem does this PR solve?

Issue Number: close #48324

Problem Summary:

What is changed and how it works?

Ignore context.Cancel from grpc, then retry the subtask.

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

tiprow · 2023-11-07T03:00:47Z

Hi @ywqzzy. 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-11-07T03:16:43Z

Codecov Report

Merging #48339 (1084ad1) into master (633d223) will increase coverage by 1.3658%.
Report is 10 commits behind head on master.
The diff coverage is 50.0000%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #48339        +/-   ##
================================================
+ Coverage   71.3791%   72.7450%   +1.3658%     
================================================
  Files          1402       1425        +23     
  Lines        406780     413726      +6946     
================================================
+ Hits         290356     300965     +10609     
+ Misses        96504      93823      -2681     
+ Partials      19920      18938       -982

Flag	Coverage Δ
integration	`43.2635% <0.0000%> (?)`
unit	`71.3910% <50.0000%> (+0.0119%)`	⬆️

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

Components	Coverage Δ
dumpling	`53.9874% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`48.6675% <ø> (-4.2493%)`	⬇️

…cel_failed

ywqzzy · 2023-11-07T08:55:19Z

/retest

tiprow · 2023-11-07T08:55:40Z

@ywqzzy: 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.

…cel_failed

D3Hunter

rest lgtm

D3Hunter · 2023-11-07T12:13:14Z

pkg/disttask/framework/scheduler/scheduler.go

@@ -608,6 +610,8 @@ func (s *BaseScheduler) markSubTaskCanceledOrFailed(ctx context.Context, subtask
 		if ctx.Err() != nil && context.Cause(ctx) == ErrCancelSubtask {
 			logutil.Logger(s.logCtx).Warn("subtask canceled", zap.Error(err))
 			s.updateSubtaskStateAndError(subtask, proto.TaskStateCanceled, nil)
+		} else if status.Code(err) == codes.Canceled {


use errors.Cause to unwrap?

No, I follow the code in br/pkg/lightning/log/log.go:181.

no harm to do it, in case somewhere wrap it, ok to leave it

D3Hunter · 2023-11-08T02:38:23Z

/lgtm

ti-chi-bot · 2023-11-08T02:38:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, tangenta

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,tangenta]
~~pkg/disttask/OWNERS~~ [D3Hunter,tangenta]

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-11-08T02:38:36Z

[LGTM Timeline notifier]

Timeline:

2023-11-07 12:10:47.957787359 +0000 UTC m=+3559845.544897503: ☑️ agreed by tangenta.
2023-11-08 02:38:34.939846704 +0000 UTC m=+3611912.526956862: ☑️ agreed by D3Hunter.

ywqzzy · 2023-11-08T03:11:07Z

/retest

tiprow · 2023-11-08T03:11:30Z

@ywqzzy: 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.

D3Hunter · 2023-11-08T03:29:03Z

/hold

need to unwrap err

D3Hunter · 2023-11-08T03:36:52Z

/unhold

ywqzzy · 2023-11-08T05:11:59Z

/retest

tiprow · 2023-11-08T05:12:22Z

@ywqzzy: 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.

hawkingrei · 2023-11-08T06:29:10Z

/retest

ywqzzy · 2023-11-08T07:13:33Z

/retest

tiprow · 2023-11-08T07:13:56Z

@ywqzzy: 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.

ti-chi-bot · 2023-11-08T08:51:02Z

In response to a cherrypick label: new pull request created to branch release-7.5: #48416.

close pingcap#48324

close #48324

ywqzzy added 2 commits November 7, 2023 10:42

fix

b7384ab

fix, add test

29e8ab6

ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 7, 2023

ti-chi-bot bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Nov 7, 2023

fix

3b16d9d

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 7, 2023

ywqzzy added 3 commits November 7, 2023 14:52

add test

f7ea356

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

aeb9a5e

…cel_failed

fix

f1da6eb

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 Nov 7, 2023

fix build

0fb07a7

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

2f91a97

…cel_failed

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/needs-tests-checked labels Nov 7, 2023

tangenta approved these changes Nov 7, 2023

View reviewed changes

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

D3Hunter reviewed Nov 7, 2023

View reviewed changes

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

ti-chi-bot bot added needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. and removed do-not-merge/needs-triage-completed labels Nov 8, 2023

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2023

fix annotate

1084ad1

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2023

ti-chi-bot bot merged commit 5a458cf into pingcap:master Nov 8, 2023
13 of 16 checks passed

ti-chi-bot mentioned this pull request Nov 8, 2023

disttask: fix cancel from grpc mark subtasks as failed (#48339) #48416

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Nov 8, 2023

disttask: fix cancel from grpc mark subtasks as failed (pingcap#48339)

42b3bdf

close pingcap#48324

ti-chi-bot bot pushed a commit that referenced this pull request Nov 8, 2023

disttask: fix cancel from grpc mark subtasks as failed (#48339) (#48416)

b2abce1

close #48324

ywqzzy mentioned this pull request Nov 9, 2023

Enhance distributed task execution framework #46258

Closed

76 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disttask: fix cancel from grpc mark subtasks as failed #48339

disttask: fix cancel from grpc mark subtasks as failed #48339

ywqzzy commented Nov 7, 2023 •

edited

Loading

tiprow bot commented Nov 7, 2023

codecov bot commented Nov 7, 2023 •

edited

Loading

ywqzzy commented Nov 7, 2023

tiprow bot commented Nov 7, 2023

D3Hunter left a comment

D3Hunter Nov 7, 2023

ywqzzy Nov 7, 2023

D3Hunter Nov 8, 2023

D3Hunter commented Nov 8, 2023

ti-chi-bot bot commented Nov 8, 2023

ti-chi-bot bot commented Nov 8, 2023

ywqzzy commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

D3Hunter commented Nov 8, 2023

D3Hunter commented Nov 8, 2023

ywqzzy commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

hawkingrei commented Nov 8, 2023

ywqzzy commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

ti-chi-bot commented Nov 8, 2023

disttask: fix cancel from grpc mark subtasks as failed #48339

disttask: fix cancel from grpc mark subtasks as failed #48339

Conversation

ywqzzy commented Nov 7, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

tiprow bot commented Nov 7, 2023

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

ywqzzy commented Nov 7, 2023

tiprow bot commented Nov 7, 2023

D3Hunter left a comment

Choose a reason for hiding this comment

D3Hunter Nov 7, 2023

Choose a reason for hiding this comment

ywqzzy Nov 7, 2023

Choose a reason for hiding this comment

D3Hunter Nov 8, 2023

Choose a reason for hiding this comment

D3Hunter commented Nov 8, 2023

ti-chi-bot bot commented Nov 8, 2023

ti-chi-bot bot commented Nov 8, 2023

[LGTM Timeline notifier]

ywqzzy commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

D3Hunter commented Nov 8, 2023

D3Hunter commented Nov 8, 2023

ywqzzy commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

hawkingrei commented Nov 8, 2023

ywqzzy commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

ti-chi-bot commented Nov 8, 2023

ywqzzy commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading