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

optimize err #395

Merged
merged 19 commits into from
Jan 8, 2023
Merged

optimize err #395

merged 19 commits into from
Jan 8, 2023

Conversation

mooleetzi
Copy link
Contributor

What this PR does:

  1. replace some inappropriate fmt.Errorf with errors.New
  2. merge /pkg/datasource/sql/types/error into pkg/util/errors/

Which issue(s) this PR fixes:

Fixes #379

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


pkg/config/parser/configuration_parser.go Outdated Show resolved Hide resolved
@@ -41,7 +41,7 @@ func (g *BranchCommitResponseCodec) Decode(in []byte) interface{} {
if data.ResultCode == message.ResultCodeFailed {
data.Msg = bytes.ReadString8Length(buf)
}
data.TransactionErrorCode = serror.TransactionErrorCode(bytes.ReadByte(buf))
data.TransactionErrorCode = serror.ErrorCode(bytes.ReadByte(buf))
Copy link
Contributor

Choose a reason for hiding this comment

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

这个不用改哈,TransactionErrorCode 可以提现这个error的类型

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里也是因为transactionErr合并到了seataError

Copy link
Contributor

Choose a reason for hiding this comment

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

这里也是因为transactionErr合并到了seataError

这里为啥要合并呢?

Copy link
Contributor

Choose a reason for hiding this comment

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

TransactionErrorCode 建议保留,和java那边对应上,会更好理解,感觉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

嗯这里改回来了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里也是因为transactionErr合并到了seataError

这里为啥要合并呢?

因为err比较乱,在不同的包里定义可能后期会出现循环引用?

@@ -51,8 +51,8 @@ func TestWithFence(t *testing.T) {
return nil
},
wantErr: true,
errStr: errors.NewTccFenceError(
errors.FencePhaseError,
errStr: errors.New(
Copy link
Contributor

Choose a reason for hiding this comment

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

errors.New 会出问题吗?后文是否会用这个error类型做判断?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里的errors是项目里定义的error包,支持后面判断的

@@ -73,14 +73,14 @@ func (handler *tccFenceWrapperHandler) PrepareFence(ctx context.Context, tx *sql

err := handler.insertTCCFenceLog(tx, xid, branchId, actionName, enum.StatusTried)
if err != nil {
dbError, ok := err.(seataErrors.TccFenceError)
if ok && dbError.Code == seataErrors.TccFenceDbDuplicateKeyError {
dbError, ok := err.(seataErrors.SeataError)
Copy link
Contributor

Choose a reason for hiding this comment

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

这个类型要改吗?TccFenceError 看起来是有明确含义的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TccFenceError+transactionErr合并到了seataError;后面会再通过errcode确定err含义
合并前,transactionErr仅有一个判断,也合并到了seataError的逻辑

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2022

Codecov Report

Merging #395 (d8528f2) into master (38903f6) will decrease coverage by 0.06%.
The diff coverage is 22.80%.

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
- Coverage   40.67%   40.60%   -0.07%     
==========================================
  Files         139      138       -1     
  Lines        8732     8737       +5     
==========================================
- Hits         3552     3548       -4     
- Misses       4891     4900       +9     
  Partials      289      289              
Impacted Files Coverage Δ
pkg/datasource/sql/at.go 0.00% <0.00%> (ø)
pkg/datasource/sql/tx.go 43.75% <0.00%> (ø)
pkg/datasource/sql/types/meta.go 36.84% <0.00%> (ø)
.../sql/undo/builder/mysql_insert_undo_log_builder.go 50.48% <0.00%> (ø)
...ce/sql/undo/executor/mysql_undo_delete_executor.go 0.00% <0.00%> (ø)
...ce/sql/undo/executor/mysql_undo_insert_executor.go 0.00% <0.00%> (ø)
pkg/remoting/getty/rpc_client.go 0.00% <0.00%> (ø)
pkg/rm/rm_remoting.go 6.89% <0.00%> (+0.07%) ⬆️
pkg/rm/tcc/fence/store/db/dao/tcc_fence_db.go 56.63% <0.00%> (+0.98%) ⬆️
pkg/rm/tcc/tcc_service.go 70.80% <0.00%> (ø)
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mooleetzi
Copy link
Contributor Author

还有就是,现在errors包有俩,一个go自带的,一个是https://github.com/pkg/errors

@Issues-translate-bot
Copy link

RoBot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


Also, now there are two errors packages, one comes with go, and the other ishttps://github.com/pkg/errors

type TccFenceError struct {
Code TransactionErrorCode
type SeataError struct {
Code ErrorCode
Message string
Copy link
Contributor

Choose a reason for hiding this comment

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

我在想,这个error意义并不是很大,要不去掉?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

目前不去掉的原因是

  1. 这个error保存了parentError信息,但是没有用到parentError的地方,这一点移除没影响
  2. 再一点就是这个err会保存一些上下文,但是也没有基于上下文对error的一些判断

所以或许可以移除?

@github-actions github-actions bot removed the protocol label Dec 12, 2022
@CLAassistant
Copy link

CLAassistant commented Dec 12, 2022

CLA assistant check
All committers have signed the CLA.

pkg/config/parser/configuration_parser.go Outdated Show resolved Hide resolved
pkg/util/errors/code.go Outdated Show resolved Hide resolved
pkg/util/errors/code.go Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the config label Jan 7, 2023
@luky116 luky116 merged commit b2f03e4 into apache:master Jan 8, 2023
georgehao pushed a commit to georgehao/seata-go that referenced this pull request Feb 4, 2023
georgehao pushed a commit to georgehao/seata-go that referenced this pull request May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optimize error in the project
7 participants