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

cluster: filter region heartbeats that contain overflow flow #3415

Merged
merged 4 commits into from
Mar 12, 2021
Merged

cluster: filter region heartbeats that contain overflow flow #3415

merged 4 commits into from
Mar 12, 2021

Conversation

HunDunDM
Copy link
Member

@HunDunDM HunDunDM commented Feb 5, 2021

Signed-off-by: Zheng Xiangsheng hundundm@gmail.com

What problem does this PR solve?

What is changed and how it works?

  • Since the flow in the region heartbeat by TiKV may overflow, clean up this part of the stat.

Check List

Tests

  • Unit test

Release note

  • No release note

@HunDunDM
cluster: filter region heartbeats that contain overflow flow
da223d6 
Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>
@HunDunDM
add unit test
3cfc5ee 
Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>
@codecov
Copy link

codecov bot commented Feb 5, 2021 •
edited
Loading

Codecov Report

Merging #3415 (0b90297) into master (b1ba2d0) will increase coverage by 0.03%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3415      +/-   ##
==========================================
+ Coverage   74.92%   74.96%   +0.03%     
==========================================
  Files         244      244              
  Lines       23578    23583       +5     
==========================================
+ Hits        17666    17679      +13     
+ Misses       4325     4318       -7     
+ Partials     1587     1586       -1
Flag Coverage Δ
unittests 74.96% <50.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/core/region.go 89.74% <50.00%> (-0.59%) ⬇️
pkg/errs/errs.go 75.00% <0.00%> (-25.00%) ⬇️
pkg/etcdutil/etcdutil.go 84.70% <0.00%> (-3.53%) ⬇️
server/election/leadership.go 83.52% <0.00%> (-3.53%) ⬇️
server/member/member.go 65.21% <0.00%> (-2.72%) ⬇️
server/schedule/operator/step.go 67.44% <0.00%> (-1.67%) ⬇️
server/tso/allocator_manager.go 75.08% <0.00%> (-0.34%) ⬇️
server/server.go 71.98% <0.00%> (-0.31%) ⬇️
server/cluster/cluster.go 83.65% <0.00%> (+0.48%) ⬆️
client/base_client.go 83.23% <0.00%> (+0.57%) ⬆️
... and 6 more

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 b1ba2d0...0b90297. Read the comment docs.

@HunDunDM
fix
4646005 
Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>
@HunDunDM HunDunDM added component/cluster Cluster logic. needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. needs-cherry-pick-5.0-rc labels Feb 5, 2021
rleungx
rleungx reviewed Feb 22, 2021
View reviewed changes
server/core/region.go
// It may be caused by overflow, refer to https://github.com/tikv/pd/issues/3379.
// They need to be filtered so as not to affect downstream.
// (flow size >= 1024TB)
ImpossibleFlowSize = 1 << 50
Copy link
Member

Choose a reason for hiding this comment

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

IMO, we can directly judge if the value is less than zero and there is no need to create a variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Values very close to the upper limit of int64 are also very dangerous.

@nolouch
Copy link
Contributor

nolouch commented Mar 1, 2021

Please merge this before 5.0 release.

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 9, 2021
@HunDunDM HunDunDM added this to the v5.0.0 ga milestone Mar 12, 2021
@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • lhy1024
  • nolouch

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 12, 2021
@HunDunDM
Merge branch 'master' into negative-check
0b90297 
Signed-off-by: HunDunDM <hundundm@gmail.com>

# Conflicts:
#	server/core/region.go
@HunDunDM
Copy link
Member Author

/merge

@ti-chi-bot
Copy link
Member

@HunDunDM: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 0b90297

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 12, 2021
@ti-chi-bot ti-chi-bot merged commit 2cf8161 into tikv:master Mar 12, 2021
ti-srebot pushed a commit to ti-srebot/pd that referenced this pull request Mar 12, 2021
@HunDunDM @ti-srebot
cherry pick tikv#3415 to release-4.0
a8e0e2e 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Mar 12, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #3472

@HunDunDM HunDunDM deleted the negative-check branch March 12, 2021 10:44
HunDunDM added a commit to ti-srebot/pd that referenced this pull request May 10, 2021
@HunDunDM
cluster: filter region heartbeats that contain overflow flow (tikv#3415)
a460e56 
* cluster: filter region heartbeats that contain overflow flow

Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>

* add unit test

Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>

* fix

Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>
HunDunDM added a commit to ti-srebot/pd that referenced this pull request May 10, 2021
@HunDunDM
cluster: filter region heartbeats that contain overflow flow (tikv#3415)
2550666 
Signed-off-by: HunDunDM <hundundm@gmail.com>
ti-chi-bot pushed a commit that referenced this pull request May 10, 2021
@ti-srebot @HunDunDM
cluster: filter region heartbeats that contain overflow flow (#3415) (#…
a5addc4 
…3472)

Signed-off-by: HunDunDM <hundundm@gmail.com>

Co-authored-by: HunDunDM <hundundm@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rleungx rleungx rleungx left review comments

@nolouch nolouch nolouch approved these changes

@lhy1024 lhy1024 lhy1024 approved these changes

@disksing disksing Awaiting requested review from disksing

Assignees
No one assigned
Labels
component/cluster Cluster logic. 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
v5.0.0 ga
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@HunDunDM @nolouch @ti-chi-bot @ti-srebot @lhy1024 @rleungx