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

DM-worker: add error abstraction in pb.ProcessError #297

Merged
merged 11 commits into from
Oct 17, 2019

Conversation

amyangfei
Copy link
Contributor

@amyangfei amyangfei commented Sep 24, 2019

What problem does this PR solve?

Currently we use pb.ProcessError to pass error information among different components on the wire through gRPC, but the information includes error type and error message only, which is not very compatible with DM new error system.

What is changed and how it works?

Add more fields into pb.ProcessError and make it possible to reconstruct a *terror.Error object from pb.ProcessError

Check List

Tests

  • Unit test
  • Integration test

Code changes

Side effects

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note
    • Refine error passing mechanism in the internal system, making it more flexible to deal with internal error.

@amyangfei amyangfei added priority/normal Minor change, requires approval from ≥1 primary reviewer type/enhancement Performance improvement or refactoring 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 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Sep 24, 2019
@codecov
Copy link

codecov bot commented Sep 24, 2019

Codecov Report

Merging #297 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #297   +/-   ##
===========================================
  Coverage   60.8069%   60.8069%           
===========================================
  Files           134        134           
  Lines         15615      15615           
===========================================
  Hits           9495       9495           
  Misses         5208       5208           
  Partials        912        912

@amyangfei amyangfei added the status/PTAL This PR is ready for review. Add this label back after committing new changes label Sep 24, 2019
@csuzhangxc
Copy link
Member

@WangXiangUSTC @csuzhangxc PTAL

@WangXiangUSTC
Copy link
Contributor

/run-compatibility-test

@WangXiangUSTC
Copy link
Contributor

/run-compatibility-test pre_commit=ce5e03815c28789877c6941e98da2161a2dd1dbb

@amyangfei amyangfei added the priority/release-blocker This PR blocks a release. Please review it ASAP. label Oct 16, 2019
@amyangfei
Copy link
Contributor Author

PTAL @csuzhangxc @WangXiangUSTC

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.

rest LGTM

dm/unit/unit.go Show resolved Hide resolved
checker/checker.go Show resolved Hide resolved
dm/worker/task_checker.go Show resolved Hide resolved
dm/worker/relay.go Outdated Show resolved Hide resolved
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 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 Oct 17, 2019
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.

rest LGTM

Comment on lines 378 to 384
result := proto.Clone(st.result).(*pb.ProcessResult)
if result != nil {
for i := range result.Errors {
result.Errors[i].Error = nil
}
}
return result
Copy link
Contributor

Choose a reason for hiding this comment

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

these code is same with realRelayHolder.Result(), how about define a funcion to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refined, PTAL

@WangXiangUSTC
Copy link
Contributor

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Oct 17, 2019
@amyangfei amyangfei merged commit 0c0fb4d into pingcap:master Oct 17, 2019
@amyangfei amyangfei deleted the refine-process-error branch October 17, 2019 07:04
@sre-bot
Copy link

sre-bot commented Oct 17, 2019

cherry pick to release-1.0 failed

amyangfei added a commit to amyangfei/dm that referenced this pull request Oct 17, 2019
@amyangfei amyangfei 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 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated 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 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Oct 17, 2019
@csuzhangxc csuzhangxc added already-update-release-note The release note is updated. Add this label once the release note is updated and removed needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Nov 1, 2019
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 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 already-update-release-note The release note is updated. Add this label once the release note is updated priority/normal Minor change, requires approval from ≥1 primary reviewer priority/release-blocker This PR blocks a release. Please review it ASAP. status/LGT2 Two reviewers already commented LGTM, ready for merge type/enhancement Performance improvement or refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants