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

statistics: add more test for hot statistics #4499

Merged

ti-chi-bot merged 8 commits into tikv:master from lhy1024:add-more-test

Jan 10, 2022

Contributor

lhy1024 commented Dec 24, 2021 •

edited

Loading

Signed-off-by: lhy1024 admin@liudos.us

What problem does this PR solve?

add more test
- add checkIntervalSum to check whether interval sums are different
- add checkIntervalSumContinuous to check whether the interval sum of the peer is continuous
- add test for cool down transfer leader
some refactor
- use buildRegion
- add one heartbeat in add replica and remove replica in move peer

Check List

Tests

Unit test

Release note

 None.

Member

ti-chi-bot commented Dec 24, 2021 •

edited

Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

disksing
rleungx

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 submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot added release-note-none do-not-merge/invalid-commit-message labels

ti-chi-bot requested review from HunDunDM and rleungx

December 24, 2021 06:51

ti-chi-bot removed the do-not-merge/invalid-commit-message label

codecov bot commented Dec 27, 2021 •

edited

Loading

Codecov Report

Merging #4499 (e40dc69) into master (6f8edf2) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4499      +/-   ##
==========================================
- Coverage   74.78%   74.72%   -0.07%     
==========================================
  Files         265      265              
  Lines       27845    27845              
==========================================
- Hits        20825    20807      -18     
- Misses       5150     5163      +13     
- Partials     1870     1875       +5

Flag	Coverage Δ
unittests	`74.72% <ø> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
server/schedulers/shuffle_hot_region.go	`53.53% <0.00%> (-12.13%)`	⬇️
server/region_syncer/client.go	`79.68% <0.00%> (-3.91%)`	⬇️
server/member/member.go	`63.10% <0.00%> (-3.75%)`	⬇️
pkg/etcdutil/etcdutil.go	`85.88% <0.00%> (-2.36%)`	⬇️
server/election/leadership.go	`75.25% <0.00%> (-2.07%)`	⬇️
server/tso/tso.go	`63.06% <0.00%> (-1.71%)`	⬇️
server/server.go	`71.49% <0.00%> (-1.50%)`	⬇️
server/schedule/operator_controller.go	`82.92% <0.00%> (-1.22%)`	⬇️
server/core/storage.go	`69.31% <0.00%> (-0.76%)`	⬇️
server/cluster/cluster.go	`82.87% <0.00%> (-0.47%)`	⬇️
... and 8 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 6f8edf2...e40dc69. Read the comment docs.

rleungx reviewed

View reviewed changes

server/statistics/hot_peer_cache_test.go

- core.WithInterval(&pdpb.TimeInterval{StartTimestamp: 0, EndTimestamp: 10}),
- core.SetReadBytes(30000*10),
- core.SetReadKeys(300000*10))
+ region := buildRegion(Read, 3, 10)

Member

rleungx Dec 28, 2021

It changes the original value. Is it expected?

Contributor Author

lhy1024 Dec 28, 2021

It doesn't matter, just make sure it's hot

server/statistics/hot_peer_cache_test.go

- core.SetReportInterval(interval),
- core.SetWrittenBytes(interval*100*1024))
+ region := buildRegion(Write, 3, interval)

Member

rleungx Dec 28, 2021

ditto

server/statistics/hot_peer_cache_test.go Outdated Show resolved Hide resolved


add more test

bf57cf7

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 force-pushed the add-more-test branch from 3bb2056 to bf57cf7 Compare

December 28, 2021 10:14

ti-chi-bot added the do-not-merge/invalid-commit-message label


ref tikv#4399

51e793c

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot removed the do-not-merge/invalid-commit-message label


add test for cool down transfer leader

b8753de

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 mentioned this pull request

Refactor hot region related modules #4477

Closed

rleungx reviewed

View reviewed changes

server/statistics/hot_peer_cache_test.go

  for i := 1; i <= 200; i++ {
- checker(c, cache, region)
+ rets := checker(c, cache, region)
+ checkIntervalSumContinuous(c, intervalSums, rets, interval)

Member

rleungx Dec 29, 2021

Suggested change

 checkIntervalSumContinuous(c, intervalSums, rets, interval)

 checkIntervalSumContinuous(c, intervalSums, checker(c, cache, region), interval)

Contributor Author

lhy1024 Jan 6, 2022

it will be too long and difficult to understand.

rleungx reviewed

View reviewed changes

server/statistics/hot_peer_cache_test.go Outdated Show resolved Hide resolved

server/statistics/hot_peer_cache_test.go

		}

		// prepare with random move peer to make the interval sum of peers are different

Member

rleungx Jan 10, 2022

Does the comment need to be changed?

Contributor Author

lhy1024 Jan 10, 2022

I don't think we need to change it because the actual move peer is to add replica in the target store and then remove replica from the source store


address comment

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx approved these changes

View reviewed changes

ti-chi-bot added the status/LGT1 label

rleungx reviewed

View reviewed changes

server/statistics/hot_peer_cache_test.go Outdated Show resolved Hide resolved


address comment

bfac82d

Signed-off-by: lhy1024 <admin@liudos.us>

disksing approved these changes

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels

Contributor

disksing commented Jan 10, 2022

/merge

Member

ti-chi-bot commented Jan 10, 2022

@disksing: 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.

Member

ti-chi-bot commented Jan 10, 2022

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

Commit hash: bfac82d

ti-chi-bot added the status/can-merge label


Merge branch 'master' into add-more-test

50f8a37

Contributor Author

lhy1024 commented Jan 10, 2022

/rebuild

1 similar comment

Contributor Author

lhy1024 commented Jan 10, 2022

/rebuild

Member

HunDunDM commented Jan 10, 2022

/run-unit-test

Contributor Author

lhy1024 commented Jan 10, 2022

/rebuild

1 similar comment

Contributor Author

lhy1024 commented Jan 10, 2022

/rebuild

lhy1024 added 2 commits

January 10, 2022 19:12


Merge branch 'master' of http://github.com/tikv/pd into add-more-test

7bc7ec7

Signed-off-by: lhy1024 <admin@liudos.us>


Merge branch 'add-more-test' of github.com:lhy1024/pd into add-more-test

e40dc69

ti-chi-bot removed the status/can-merge label

Contributor Author

lhy1024 commented Jan 10, 2022

/merge

Member

ti-chi-bot commented Jan 10, 2022

@lhy1024: 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.

Member

ti-chi-bot commented Jan 10, 2022

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

Commit hash: e40dc69

ti-chi-bot added the status/can-merge label

ti-chi-bot merged commit cfb909d into tikv:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note-none status/can-merge status/LGT2