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: no longer substring the message in rpc transmission #3201

Merged
merged 14 commits into from
Oct 22, 2020

Conversation

l81893521
Copy link
Contributor

@l81893521 l81893521 commented Oct 16, 2020

Ⅰ. Describe what this PR did

User Feedback: Misunderstand the message when transaction error occurred in TC. Because if the message longer than 128 byte, and TC would substring the message before rpc transmission.

no longer substring the message in rpc transmission and show the hold message especially in transaction error.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Copy link
Contributor

@wangliang181230 wangliang181230 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@lightClouds917 lightClouds917 left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-io
Copy link

codecov-io commented Oct 16, 2020

Codecov Report

Merging #3201 into develop will decrease coverage by 0.12%.
The diff coverage is 33.33%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #3201      +/-   ##
=============================================
- Coverage      51.05%   50.93%   -0.13%     
+ Complexity      3248     3246       -2     
=============================================
  Files            608      608              
  Lines          19973    19946      -27     
  Branches        2494     2446      -48     
=============================================
- Hits           10197    10159      -38     
+ Misses          8762     8752      -10     
- Partials        1014     1035      +21     
Impacted Files Coverage Δ Complexity Δ
...zer/seata/protocol/AbstractResultMessageCodec.java 77.77% <33.33%> (+2.77%) 5.00 <0.00> (ø)
...obuf/convertor/BranchRegisterRequestConvertor.java 90.47% <0.00%> (-9.53%) 3.00% <0.00%> (ø%)
...otobuf/convertor/BranchReportRequestConvertor.java 90.90% <0.00%> (-9.10%) 3.00% <0.00%> (ø%)
...obuf/convertor/BranchRollbackRequestConvertor.java 92.00% <0.00%> (-8.00%) 3.00% <0.00%> (ø%)
...otobuf/convertor/GlobalBeginResponseConvertor.java 92.59% <0.00%> (-7.41%) 3.00% <0.00%> (ø%)
...otobuf/convertor/GlobalCommitRequestConvertor.java 93.75% <0.00%> (-6.25%) 3.00% <0.00%> (ø%)
...otobuf/convertor/GlobalStatusRequestConvertor.java 93.75% <0.00%> (-6.25%) 3.00% <0.00%> (ø%)
...obuf/convertor/GlobalRollbackRequestConvertor.java 93.75% <0.00%> (-6.25%) 3.00% <0.00%> (ø%)
...protobuf/convertor/RegisterTMRequestConvertor.java 94.73% <0.00%> (-5.27%) 3.00% <0.00%> (ø%)
...protobuf/convertor/RegisterRMRequestConvertor.java 90.47% <0.00%> (-4.77%) 3.00% <0.00%> (ø%)
... and 28 more

@jsbxyyx
Copy link
Member

jsbxyyx commented Oct 16, 2020

We recommend keeping the original situation. for example: adding go to seata-server view log in message.

@funky-eyes funky-eyes added this to the 1.4.0 milestone Oct 21, 2020
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

LGTM

@slievrly slievrly merged commit 0e38080 into apache:develop Oct 22, 2020
@l81893521 l81893521 deleted the optimize_substring_msg branch October 23, 2020 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants