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.

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

Add warp and cause interface #23

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

imtbkcat
Copy link

@imtbkcat imtbkcat commented Aug 7, 2020

  • Add Warp and Cause interface to save cause error.
  • Add FastGenWithCause to generate error with error code and cause error

terror_error.go Outdated
@@ -291,3 +296,35 @@ func (e *Error) UnmarshalJSON(data []byte) error {
}
return nil
}

func (e *Error) WarpCauseError(err error) *Error {
Copy link

Choose a reason for hiding this comment

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

Prefer to use a shorter method name Wrap (and there is a typo in your signature).

terror_error.go Outdated
}

func (e *Error) Cause() error {
return e.cause
Copy link

Choose a reason for hiding this comment

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

The cause error may be another wrapper for the real root cause. Do we need to handle this situation?

// Class returns ErrClass
func (e *Error) Class() *ErrClass {
return e.class
// Cause is used to warp some third party error.
Copy link

Choose a reason for hiding this comment

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

typo

// Error implements error interface.
func (e *Error) Error() string {
describe := e.codeText
if len(describe) == 0 {
describe = ErrCodeText(strconv.Itoa(int(e.code)))
}
return fmt.Sprintf("[%s] %s", e.RFCCode(), e.getMsg())
return fmt.Sprintf("[%s] %s", e.RFCCode(), e.GetMsg())
Copy link

Choose a reason for hiding this comment

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

I think we should use the local variable describe as the prefix of the error message.

return nil
}

func (e *Error) Wrap(err error) *Error {
e.cause = err
Copy link

Choose a reason for hiding this comment

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

I think we should return a new instance of when Wrap a message instead of in-place changing the original instance. The error will be a global instance in many cases.

@@ -84,23 +93,7 @@ func (e *Error) Code() ErrCode {
// The error code is a 3-tuple of abbreviated component name, error class and error code,
// joined by a colon like {Component}:{ErrorClass}:{InnerErrorCode}.
func (e *Error) RFCCode() RFCErrorCode {
ec := e.Class()
Copy link

Choose a reason for hiding this comment

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

We should return the e.codeText if the e.codeText had been assigned a value.

terror_error.go Outdated

// SetWorkaround sets the workaround for standard error.
func (e *Error) SetWorkaround(workaround string) *Error {
e.Workaround = workaround
Copy link

Choose a reason for hiding this comment

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

Can we allow users to dynamically change the fields?

Copy link
Author

Choose a reason for hiding this comment

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

This is a decorator function. Since old error in TiDB do not have workaround field. Using this method can add workaround with miniumn change.

Copy link

Choose a reason for hiding this comment

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

Reference to the demo https://play.golang.org/p/rJBVbbkbXLP, we can minimize the change and make the error immutable.

return zap.Field{Key: "error", Type: zapcore.ErrorType, Interface: err.FastGenWithCause()}
}

// CauseError returns zap.Field contains error.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// CauseError returns zap.Field contains error.
// DetailError returns zap.Field contains error.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, I think we can combine these two methods? And Wrap the cause error inside of this function so that we can directly use something like CauseError(err, causeErr).

Comment on lines +354 to +361
func CauseError(err *Error) zap.Field {
return zap.Field{Key: "error", Type: zapcore.ErrorType, Interface: err.FastGenWithCause()}
}

// CauseError returns zap.Field contains error.
func DetailError(err *Error) zap.Field {
return zap.Field{Key: "error", Type: zapcore.ErrorType, Interface: err.FastGenByArgs()}
}
Copy link

Choose a reason for hiding this comment

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

According to discussion with @rleungx, we can remove this two functions and use zap.Error directly. And PD will customize a function to adopt their scenarios.


func (e *Error) FastGenWithCause(args ...interface{}) error {
err := *e
err.message = e.cause.Error()

Choose a reason for hiding this comment

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

will panic here is Wrap a nil error


func (e *Error) GenWithStackByCause(args ...interface{}) error {
err := *e
err.message = e.cause.Error()

Choose a reason for hiding this comment

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

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants