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

Fix the numeric overflow bug of ping response data in the cluster module #844

Merged
merged 1 commit into from
Jun 18, 2019

Conversation

sczyh30
Copy link
Member

@sczyh30 sczyh30 commented Jun 17, 2019

Signed-off-by: Eric Zhao sczyh16@gmail.com

Describe what this PR does / why we need it

Fix the bug that the ping response data (of byte type) in the default cluster transport implementation might overflow.

Does this pull request fix one issue?

Fixes #843

Describe how you did it

Change type of cluster ping data response from byte to int. Both PingResponseDataWriter and PingResponseDataDecoder have been modified.

The PingResponseDataDecoder will be compatible with old versions:

  • For old token servers and new clients, the client will parse the ping response as byte.
  • Old clients will be compatible with token servers, but overflow will still happen.

Describe how to verify it

Run the test cases.

Special notes for reviews

NONE

- this change is compatible with old versions

Signed-off-by: Eric Zhao <sczyh16@gmail.com>
@sczyh30 sczyh30 added to-review To review area/cluster-flow Issues or PRs related to cluster flow control labels Jun 17, 2019
@sczyh30 sczyh30 added this to the 1.7.0 milestone Jun 17, 2019
@codecov-io
Copy link

Codecov Report

Merging #844 into master will increase coverage by 0.08%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #844      +/-   ##
============================================
+ Coverage     41.73%   41.82%   +0.08%     
- Complexity     1383     1389       +6     
============================================
  Files           305      305              
  Lines          8807     8810       +3     
  Branches       1188     1190       +2     
============================================
+ Hits           3676     3685       +9     
+ Misses         4677     4670       -7     
- Partials        454      455       +1
Impacted Files Coverage Δ Complexity Δ
...ster/server/codec/data/PingResponseDataWriter.java 60% <100%> (+60%) 2 <0> (+2) ⬆️
...ter/client/codec/data/PingResponseDataDecoder.java 71.42% <75%> (+71.42%) 3 <2> (+3) ⬆️
...ava/com/alibaba/csp/sentinel/node/ClusterNode.java 100% <0%> (+4.76%) 8% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 254ccbb...7452826. Read the comment docs.

Copy link
Contributor

@CarpenterLee CarpenterLee left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cluster-flow Issues or PRs related to cluster flow control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ping response data (byte type) in default cluster module might overflow
3 participants