Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

error_message: remove stack error message #746

Merged
merged 12 commits into from
Jun 18, 2020

Conversation

GMHDBJD
Copy link
Collaborator

@GMHDBJD GMHDBJD commented Jun 16, 2020

What problem does this PR solve?

remove stack information of error message

What is changed and how it works?

  • replace errors.ErrorStack with terror.Message

Tests

  • Unit test
  • Integration test

@GMHDBJD GMHDBJD added status/WIP This PR is still work in progress needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 labels Jun 16, 2020
@GMHDBJD GMHDBJD added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Jun 16, 2020
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jun 16, 2020

Test fail because pingcap/tidb#18061

@csuzhangxc
Copy link
Member

/run-all-tests tidb=v4.0.1

@csuzhangxc
Copy link
Member

/run-all-tests tidb=v4.0.0

@@ -44,7 +44,7 @@ func checkTaskFunc(cmd *cobra.Command, _ []string) {
}
content, err := common.GetFileContent(cmd.Flags().Arg(0))
if err != nil {
common.PrintLines("get file content error:\n%v", errors.ErrorStack(err))
common.PrintLines("get file content error:\n%v", terror.Message(err))
Copy link
Member

Choose a reason for hiding this comment

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

How about returning Error directly? for terror, it will contain useful code, class, etc.

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jun 17, 2020

/run-all-tests tidb=v4.0.0

1 similar comment
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jun 17, 2020

/run-all-tests tidb=v4.0.0

@GMHDBJD GMHDBJD added status/WIP This PR is still work in progress and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jun 17, 2020
@csuzhangxc csuzhangxc added this to the v1.0.7 milestone Jun 18, 2020
@GMHDBJD GMHDBJD force-pushed the removeStackErrMsg branch from 8f7c249 to e78ee18 Compare June 18, 2020 02:37
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jun 18, 2020

/run-all-tests tidb=v4.0.0

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jun 18, 2020

/run-all-tests tidb=v4.0.0

@GMHDBJD GMHDBJD added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Jun 18, 2020
dm/ctl/common/util.go Outdated Show resolved Hide resolved
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jun 18, 2020

/run-all-tests tidb=v4.0.0

Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jun 18, 2020
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Jun 18, 2020

/run-all-tests tidb=v4.0.0

@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #746 into master will increase coverage by 0.0267%.
The diff coverage is 10.5263%.

@@               Coverage Diff                @@
##             master       #746        +/-   ##
================================================
+ Coverage   57.0871%   57.1138%   +0.0266%     
================================================
  Files           206        205         -1     
  Lines         21292      21191       -101     
================================================
- Hits          12155      12103        -52     
+ Misses         7951       7916        -35     
+ Partials       1186       1172        -14     

Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Jun 18, 2020
@GMHDBJD GMHDBJD merged commit fb52a1f into pingcap:master Jun 18, 2020
@ti-srebot
Copy link

cherry pick to release-1.0 failed

GMHDBJD added a commit to GMHDBJD/dm that referenced this pull request Jun 18, 2020
@csuzhangxc csuzhangxc added already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 labels Jun 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated status/LGT2 Two reviewers already commented LGTM, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants