disttask: make task executor onerror log skip one more level of stack.

[LindaSummer]

What problem does this PR solve?

Issue Number: close #56014

Problem Summary:

What changed and how does it work?

Error log in onError function's stack is always in the same function.

But the real stack should be the one invoking onError.

Check List

Tests

• Unit test

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

[ti-chi-bot]

Hi @LindaSummer. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[tiprow]

Hi @LindaSummer. 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-sigs/prow repository.

[LindaSummer]

Hi @lance6716 ,

Please take a look. 😊

Thanks very much.

Best Regards,
Edward

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 56.2016%. Comparing base (65fcaec) to head (3f3fc02).

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #56618         +/-   ##
=================================================
- Coverage   73.3740%   56.2016%   -17.1724%     
=================================================
  Files          1630       1754        +124     
  Lines        450230     629127     +178897     
=================================================
+ Hits         330352     353580      +23228     
- Misses        99584     251608     +152024     
- Partials      20294      23939       +3645     

Flag	Coverage Δ
integration	36.7676% <20.0000%> (?)
unit	72.4536% <66.6666%> (-0.0525%)	⬇️

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

Components	Coverage Δ
dumpling	52.9478% <ø> (ø)
parser	∅ <ø> (∅)
br	52.3696% <ø> (+6.8134%)	⬆️

[LindaSummer]

/ok-to-test

And please post the test result. Or if you can, add an unit test for it. For example, check the output of logger and verify it contents the function that throws the error

Thanks very much for your suggestions!

I will try to add a test case for it. 😊

Best Regards,
Edward

[LindaSummer]

Hi @lance6716 ,

I have corrected my mistake and add unit tests. 😊

Please take a look.

Best Regards,
Edward

[lance6716]

Please run make bazel_prepare to update .bazel files, or just apply the following git diff (run git apply - and paste following contents):

diff --git a/pkg/disttask/framework/taskexecutor/BUILD.bazel b/pkg/disttask/framework/taskexecutor/BUILD.bazel
index e65cc685e0..79a2483481 100644
--- a/pkg/disttask/framework/taskexecutor/BUILD.bazel
+++ b/pkg/disttask/framework/taskexecutor/BUILD.bazel
@@ -51,7 +51,7 @@ go_test(
     ],
     embed = [":taskexecutor"],
     flaky = True,
-    shard_count = 16,
+    shard_count = 17,
     deps = [
         "//pkg/disttask/framework/mock",
         "//pkg/disttask/framework/mock/execute",
@@ -74,5 +74,7 @@ go_test(
         "@org_golang_google_grpc//status",
         "@org_uber_go_goleak//:goleak",
         "@org_uber_go_mock//gomock",
+        "@org_uber_go_zap//:zap",
+        "@org_uber_go_zap//zaptest/observer",
     ],
 )

Or I can help you push that change if you have some problems.

[LindaSummer]

Thanks very much for your warm suggestion and help!

I will update this PR later today.😄

Best Regards,
Edward

[LindaSummer]

Hi @lance6716 ,

Thanks very much for your bazel patch!😊

I have updated related code and added stacktrace explicitly for all onError invokers.

Best Regards,
Edward

[LindaSummer]

Hi @lance6716 ，

Thanks for your patience and warm help! 😊

I have added an error stack field for stack info inside error and skipped it for error without stack info.

Best Regards,
Edward

[LindaSummer]

/retest

[lance6716]

rest lgtm

[LindaSummer]

rest lgtm

Thanks very much for your patience and help! 😊

I'll update it later today.

Best Regards,
Edward

[LindaSummer]

/retest

[LindaSummer]

Hi @lance6716 ,

Thanks for your review and suggestions! 😊

I have updated the related code, please help take a look.

Best Regards,
Edward

[lance6716]

/cc @GMHDBJD

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716

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:

• pkg/disttask/OWNERS [lance6716]

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]

[LGTM Timeline notifier]

Timeline:

• 2024-10-17 13:49:07.952007338 +0000 UTC m=+538145.100917175: ☑️ agreed by lance6716.

[LindaSummer]

/retest