server: pd can support bucket steam and save them. #4670

bufferflies · 2022-02-23T03:01:57Z

What problem does this PR solve?

Issue Number: Close #4669

What is changed and how it works?

Check List

Tests

Unit test

Code changes

Has configuration change

Side effects

Related changes

Release note

None.

ti-chi-bot · 2022-02-23T03:01:58Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
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.

codecov · 2022-02-23T03:10:29Z

Codecov Report

Merging #4670 (23dbb43) into master (c408535) will decrease coverage by 0.41%.
The diff coverage is 24.46%.

@@            Coverage Diff             @@
##           master    #4670      +/-   ##
==========================================
- Coverage   75.42%   75.00%   -0.42%     
==========================================
  Files         294      294              
  Lines       28334    28463     +129     
==========================================
- Hits        21371    21349      -22     
- Misses       5078     5213     +135     
- Partials     1885     1901      +16

Flag	Coverage Δ
unittests	`75.00% <24.46%> (-0.42%)`	⬇️

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

Impacted Files	Coverage Δ
server/cluster/cluster_worker.go	`71.29% <0.00%> (-0.67%)`	⬇️
server/core/basic_cluster.go	`88.70% <0.00%> (-2.96%)`	⬇️
server/grpc_service.go	`50.68% <0.00%> (-5.14%)`	⬇️
server/cluster/cluster.go	`84.82% <86.66%> (+0.01%)`	⬆️
server/cluster/metrics.go	`100.00% <100.00%> (ø)`
server/core/region.go	`91.78% <100.00%> (+0.19%)`	⬆️
server/metrics.go	`100.00% <100.00%> (ø)`
pkg/tempurl/tempurl.go	`45.00% <0.00%> (-15.00%)`	⬇️
server/tso/local_allocator.go	`64.86% <0.00%> (-6.76%)`	⬇️
server/tso/allocator_manager.go	`62.56% <0.00%> (-4.82%)`	⬇️
... and 13 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 226b25b...23dbb43. Read the comment docs.

bufferflies · 2022-03-09T09:11:23Z

PTAL @lhy1024 @rleungx

bufferflies · 2022-03-09T09:13:50Z

/open

server/cluster/cluster.go

rleungx · 2022-03-10T11:13:33Z

server/cluster/cluster_worker.go

+
+// HandleBucketHeartbeat processes RegionInfo reports from client
+func (c *RaftCluster) HandleBucketHeartbeat(buckets *metapb.Buckets) error {
+ return c.processBucketHeartbeat(buckets)


Why do we need this abstruction?

In the first, HandleBucketHeartbeat should handle the change of the bucket meta(keys and version), but in the second , it will handle the statistics.

server/core/region.go

rleungx · 2022-03-10T11:18:50Z

The implementation is almost the same as region heartbeat, can we abstract the common part to reduce code?

Signed-off-by: bufferflies <1045931706@qq.com>

JmPotato · 2022-03-30T02:29:05Z

server/cluster/cluster.go

+ }
+
+ // region should not update if the version of the buckets is less than the old one.
+ if old := region.GetBuckets(); old != nil && old.Version >= buckets.Version {


Shouldn't it be old.Version > buckets.Version?

region will not update if version is not change.

It seems the region heartbeat only checks < here. As for a region, the version only changes when splitting or merging happens, so it's still necessary to update other statistical information while the version remains the same. I assume a bucket should also work like that, right?

pd/server/core/basic_cluster.go

Line 362 in f605a2c

if region.GetRegionEpoch().GetVersion() < item.GetRegionEpoch().GetVersion() && !isRegionRecreated(region) {

Is there a possibility that we have an ABA problem here? Consider the following case:
There are two requests coming at the same time. Both request A and request B have passed the version check, A is a little bit earlier than B. But B which has a larger version is finished earlier than A. After A is finished, B is lost.

It seems the region heartbeat only checks < here. As for a region, the version only changes when splitting or merging happens, so it's still necessary to update other statistical information while the version remains the same. I assume a bucket should also work like that, right?

pd/server/core/basic_cluster.go

Line 362 in f605a2c

if region.GetRegionEpoch().GetVersion() < item.GetRegionEpoch().GetVersion() && !isRegionRecreated(region) {

the version of the bucket maybe happened at cron job or wirte skew, so it should drop the buckets info if the version is same.

Is there a possibility that we have an ABA problem here? Consider the following case: There are two requests coming at the same time. Both request A and request B have passed the version check, A is a little bit earlier than B. But B which has a larger version is finished earlier than A. After A is finished, B is lost.

yes, the conor will be happened , I will use CAS to avoid the latest buckets to be replaced.

server/cluster/cluster_test.go

server/core/region.go

JmPotato · 2022-03-30T02:32:20Z

server/core/region.go

- if r.approximateSize != 0 {
- return
+func (r *RegionInfo) Inherit(origin *RegionInfo) {
+ // regionSize should not be zero if region is not empty.


I think we can check whether the origin is nil at the beginning of this function and return directly if it is.

region size will be 1 if origin is nil and the size of the new region is zero.

server/grpc_service.go

Signed-off-by: bufferflies <1045931706@qq.com>

server/cluster/cluster.go

rleungx

Mostly LGTM. And we need a concurrency test for this PR.

Signed-off-by: bufferflies <1045931706@qq.com>

bufferflies · 2022-03-31T09:21:39Z

Mostly LGTM. And we need a concurrency test for this PR.

I have added one concurrency test for ABA and one benchmark test for region update.

bufferflies · 2022-04-01T02:37:36Z

/merge

ti-chi-bot · 2022-04-01T02:37:37Z

@bufferflies: 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 · 2022-04-01T02:37:39Z

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

Commit hash: 23dbb43

ref #4670, close #4749 Signed-off-by: bufferflies <1045931706@qq.com>

ti-chi-bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Feb 23, 2022

ti-chi-bot requested review from HunDunDM and rleungx February 23, 2022 03:02

bufferflies force-pushed the feature/bucket branch 5 times, most recently from cf7f152 to cdc56d9 Compare February 25, 2022 02:28

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 25, 2022

bufferflies changed the title ~~draft: pd can handler bucket steam and support searching function.~~ server: pd can handler bucket steam and support searching function. Mar 3, 2022

bufferflies removed the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Mar 3, 2022

bufferflies changed the title ~~server: pd can handler bucket steam and support searching function.~~ server: pd can support bucket steam and save them. Mar 4, 2022

ti-chi-bot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Mar 4, 2022

bufferflies requested a review from lhy1024 March 4, 2022 01:53

bufferflies force-pushed the feature/bucket branch from 06236c9 to ea4c1d7 Compare March 4, 2022 09:31

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 7, 2022

bufferflies force-pushed the feature/bucket branch from 36d68c7 to f9140d0 Compare March 8, 2022 11:00

bufferflies closed this Mar 9, 2022

bufferflies force-pushed the feature/bucket branch from 94751cb to 2e22f1f Compare March 9, 2022 09:12

bufferflies reopened this Mar 9, 2022

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2022

nolouch reviewed Mar 10, 2022

View reviewed changes

server/cluster/cluster.go Show resolved Hide resolved

rleungx reviewed Mar 10, 2022

View reviewed changes

bucket

257a060

Signed-off-by: bufferflies <1045931706@qq.com>

bufferflies force-pushed the feature/bucket branch from 7d81749 to b20dd37 Compare March 29, 2022 10:19

JmPotato reviewed Mar 30, 2022

View reviewed changes

rleungx reviewed Mar 30, 2022

View reviewed changes

server/grpc_service.go Outdated Show resolved Hide resolved

bufferflies force-pushed the feature/bucket branch from 23eb75a to 7ef9147 Compare March 30, 2022 06:07

grafana add store infomation

a78e632

Signed-off-by: bufferflies <1045931706@qq.com>

bufferflies force-pushed the feature/bucket branch from 7ef9147 to a78e632 Compare March 30, 2022 12:44

bufferflies requested review from rleungx and JmPotato March 31, 2022 03:41

bufferflies force-pushed the feature/bucket branch from a3acd39 to 93976a9 Compare March 31, 2022 07:27

solve ABA

d34547c

Signed-off-by: bufferflies <1045931706@qq.com>

bufferflies force-pushed the feature/bucket branch from 93976a9 to d34547c Compare March 31, 2022 08:02

rleungx reviewed Mar 31, 2022

View reviewed changes

server/cluster/cluster.go Outdated Show resolved Hide resolved

rleungx approved these changes Mar 31, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 31, 2022

bufferflies force-pushed the feature/bucket branch from 0ac9ced to f2d0db6 Compare March 31, 2022 09:10

add ConcurrentReportBucket

9d481c8

Signed-off-by: bufferflies <1045931706@qq.com>

bufferflies force-pushed the feature/bucket branch from f2d0db6 to 9d481c8 Compare March 31, 2022 09:20

JmPotato approved these changes Mar 31, 2022

View reviewed changes

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 31, 2022

Merge branch 'master' into feature/bucket

23dbb43

ti-chi-bot merged commit 44a8672 into tikv:master Apr 1, 2022

ti-chi-bot pushed a commit that referenced this pull request Apr 14, 2022

region_sync: region syncer support bucket (#4750)

4f9369f

ref #4670, close #4749 Signed-off-by: bufferflies <1045931706@qq.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: pd can support bucket steam and save them. #4670

server: pd can support bucket steam and save them. #4670

bufferflies commented Feb 23, 2022 •

edited by JmPotato

Loading

ti-chi-bot commented Feb 23, 2022 •

edited

Loading

codecov bot commented Feb 23, 2022 •

edited

Loading

bufferflies commented Mar 9, 2022

bufferflies commented Mar 9, 2022

rleungx Mar 10, 2022

bufferflies Mar 11, 2022

rleungx commented Mar 10, 2022

JmPotato Mar 30, 2022

bufferflies Mar 30, 2022

JmPotato Mar 31, 2022

rleungx Mar 31, 2022 •

edited

Loading

bufferflies Mar 31, 2022

bufferflies Mar 31, 2022

JmPotato Mar 30, 2022

bufferflies Mar 30, 2022

rleungx left a comment

bufferflies commented Mar 31, 2022

bufferflies commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

server: pd can support bucket steam and save them. #4670

server: pd can support bucket steam and save them. #4670

Conversation

bufferflies commented Feb 23, 2022 • edited by JmPotato Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Feb 23, 2022 • edited Loading

codecov bot commented Feb 23, 2022 • edited Loading

Codecov Report

bufferflies commented Mar 9, 2022

bufferflies commented Mar 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx commented Mar 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx left a comment

Choose a reason for hiding this comment

bufferflies commented Mar 31, 2022

bufferflies commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

ti-chi-bot commented Apr 1, 2022

bufferflies commented Feb 23, 2022 •

edited by JmPotato

Loading

ti-chi-bot commented Feb 23, 2022 •

edited

Loading

codecov bot commented Feb 23, 2022 •

edited

Loading

rleungx Mar 31, 2022 •

edited

Loading