Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

*: optimize log and make their specification consistent #2853

Merged
merged 8 commits into from
Sep 1, 2020
Merged

*: optimize log and make their specification consistent #2853

merged 8 commits into from
Sep 1, 2020

Conversation

rleungx
Copy link
Member

@rleungx rleungx commented Aug 27, 2020

What problem does this PR solve?

Related to #2704.

What is changed and how it works?

This PR does some optimization for the error log by defining the error when we first meet it.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

Release note

@rleungx rleungx added the component/log Log. label Aug 27, 2020
rleungx
rleungx commented Aug 27, 2020
View reviewed changes
client/base_client.go Outdated Show resolved Hide resolved
@rleungx
Copy link
Member Author

rleungx commented Aug 28, 2020

/run-all-tests

1 similar comment
@rleungx
Copy link
Member Author

rleungx commented Aug 28, 2020

/run-all-tests

JmPotato
JmPotato approved these changes Aug 29, 2020
View reviewed changes
Copy link
Member

@JmPotato JmPotato left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 29, 2020
@rleungx
Copy link
Member Author

rleungx commented Aug 31, 2020

/run-all-tests

2 similar comments
@rleungx
Copy link
Member Author

rleungx commented Aug 31, 2020

/run-all-tests

@rleungx
Copy link
Member Author

rleungx commented Aug 31, 2020

/run-all-tests

@rleungx
Copy link
Member Author

rleungx commented Aug 31, 2020

/rebuild

@rleungx rleungx force-pushed the optimize-log branch 2 times, most recently from 18b362f to 1712dda Compare August 31, 2020 09:14
howardlau1999
howardlau1999 reviewed Sep 1, 2020
View reviewed changes
pkg/errs/errno.go Outdated Show resolved Hide resolved
pkg/errs/errno.go Outdated Show resolved Hide resolved
lhy1024
lhy1024 reviewed Sep 1, 2020
View reviewed changes
pkg/apiutil/serverapi/middleware.go
continue
}

b, err := ioutil.ReadAll(resp.Body)
resp.Body.Close()
if err != nil {
log.Error("read failed", errs.ZapError(errs.ErrReadHTTPBody, err))
log.Error("read failed", errs.ZapError(errs.ErrIORead, err))
Copy link
Contributor

Choose a reason for hiding this comment

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

why change its name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's an IO error

client/base_client.go Show resolved Hide resolved
pkg/errs/errno.go
// tso errors
var (
ErrInvalidTimestamp = errors.Normalize("invalid timestamp", errors.RFCCodeText("PD:tso:ErrInvalidTimestamp"))
ErrLogicOverflow = errors.Normalize("logic part overflow", errors.RFCCodeText("PD:tso:ErrLogicOverflow"))
ErrIncorrectSystemTime = errors.Normalize("incorrect system time", errors.RFCCodeText("PD:tso:ErrIncorrectSystemTime"))
)

// adapter errors
// member errors
Copy link
Contributor

Choose a reason for hiding this comment

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

On what basis do we rank these errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO, we just need to distinguish the errors generated in PD internal or in the third-party package. There is no rank concept.

server/election/leadership.go Show resolved Hide resolved
server/kv/etcd_kv.go Show resolved Hide resolved
@rleungx
address comments
6e08a84 
Signed-off-by: Ryan Leung <[email protected]>
howardlau1999
howardlau1999 reviewed Sep 1, 2020
View reviewed changes
server/election/leadership.go Outdated
@@ -34,7 +34,7 @@ func GetLeader(c *clientv3.Client, leaderPath string) (*pdpb.Member, int64, erro
leader := &pdpb.Member{}
ok, rev, err := etcdutil.GetProtoMsgWithModRev(c, leaderPath, leader)
if err != nil {
return nil, 0, err
return nil, 0, errs.ErrEtcdGetProtoMsgWithModRev.Wrap(err).FastGenWithCause()
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should handle errors in the lower level

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

server/kv/etcd_kv.go
}
if !resp.Succeeded {
return errors.WithStack(errTxnFailed)
return errs.ErrEtcdTxn.FastGenByArgs()
Copy link
Contributor

Choose a reason for hiding this comment

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

This error has a %s placeholder. is it ok to pass no argument?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@rleungx
address comments
b85e4ac 
Signed-off-by: Ryan Leung <[email protected]>
lhy1024
lhy1024 reviewed Sep 1, 2020
View reviewed changes
server/member/member.go Outdated
Comment on lines 160 to 161
err := errs.ErrEtcdLeaderNotFound.FastGenByArgs()
log.Error("no etcd leader, check pd leader later", errs.ZapError(err))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err := errs.ErrEtcdLeaderNotFound.FastGenByArgs()
log.Error("no etcd leader, check pd leader later", errs.ZapError(err))
log.Error("no etcd leader, check pd leader later", errs.ZapError(errs.ErrEtcdLeaderNotFound))

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@rleungx
address comments
65c448c 
Signed-off-by: Ryan Leung <[email protected]>
@ti-srebot
Copy link
Contributor

@howardlau1999,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: scheduling(slack).

@rleungx
Copy link
Member Author

rleungx commented Sep 1, 2020

@lhy1024 PTAL

@rleungx
Copy link
Member Author

rleungx commented Sep 1, 2020

/run-all-tests

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 1, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 1, 2020
@lhy1024
Copy link
Contributor

lhy1024 commented Sep 1, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 1, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 6876790 into tikv:master Sep 1, 2020
howardlau1999 pushed a commit to howardlau1999/pd that referenced this pull request Sep 2, 2020
@rleungx
*: optimize log and make their specification consistent (tikv#2853)
4eb2618 
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Howard Lau <[email protected]>
@JmPotato JmPotato mentioned this pull request Sep 2, 2020
Merged
@rleungx rleungx added the needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. label Sep 4, 2020
@rleungx rleungx added this to the v4.0.6 milestone Sep 4, 2020
ti-srebot pushed a commit to ti-srebot/pd that referenced this pull request Sep 4, 2020
@rleungx @ti-srebot
cherry pick tikv#2853 to release-4.0
83e156d 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Sep 4, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #2903

@rleungx rleungx removed this from the v4.0.6 milestone Sep 4, 2020
rleungx added a commit to ti-srebot/pd that referenced this pull request Sep 7, 2020
@rleungx
cherry pick tikv#2853 to release-4.0
6756173 
Signed-off-by: ti-srebot <[email protected]>
rleungx added a commit to ti-srebot/pd that referenced this pull request Sep 7, 2020
@rleungx
cherry pick tikv#2853 to release-4.0
49fd607 
Signed-off-by: ti-srebot <[email protected]>
rleungx added a commit to ti-srebot/pd that referenced this pull request Sep 7, 2020
@rleungx
cherry pick tikv#2853 to release-4.0
3c75a25 
Signed-off-by: Ryan Leung <[email protected]>
rleungx added a commit to ti-srebot/pd that referenced this pull request Sep 7, 2020
@rleungx
cherry pick tikv#2853 to release-4.0
0bbdfd1 
Signed-off-by: Ryan Leung <[email protected]>
rleungx added a commit to ti-srebot/pd that referenced this pull request Sep 7, 2020
@rleungx
cherry pick tikv#2853 to release-4.0
49aa57e 
Signed-off-by: Ryan Leung <[email protected]>
rleungx added a commit to ti-srebot/pd that referenced this pull request Sep 7, 2020
@rleungx
cherry pick tikv#2853 to release-4.0
ba4b014 
Signed-off-by: Ryan Leung <[email protected]>
ti-srebot added a commit that referenced this pull request Sep 7, 2020
@ti-srebot
*: optimize log and make their specification consistent (#2853) (#2903)
d8671fe 
Signed-off-by: Ryan Leung <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Sep 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ti-srebot ti-srebot ti-srebot approved these changes

@lhy1024 lhy1024 lhy1024 approved these changes

@howardlau1999 howardlau1999 howardlau1999 approved these changes

@JmPotato JmPotato JmPotato approved these changes

@nolouch nolouch Awaiting requested review from nolouch

@HunDunDM HunDunDM Awaiting requested review from HunDunDM

Assignees
No one assigned
Labels
component/log Log. needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rleungx @ti-srebot @lhy1024 @JmPotato @howardlau1999