server/cache: reorganize the cache #359

huachaohuang · 2016-10-23T05:33:45Z

First, stores and regions are reorganized so that we can easily access
the information we need.

Second, the algorithm to handle region heartbeat is modified
accordingly, to handle Split and Merge.

It's hard to handle Split and Merge perfectly to keep the cache always
reflecting the newest information about which region is serving which
range. But we can keep the cache always updating according to the region
heartbeat.

Here's how we handle region heartbeat. First, if the region does not
exist, it is added to the cache directly. Second, if the region exists
but the heartbeat is stale, an error will be returned. Finally, if the
region exists and the heartbeat is not stale, it is updated to the
cache. Additionaly, every time a region is added or updated, all range
overlap regions will be deleted from the search tree first, and then
the region will be inserted. Note that regions deleted from the search
tree can still be found by the region id.

This PR relies on #353 and #356.

huachaohuang · 2016-10-26T06:46:04Z

PTAL @siddontang @overvenus
cache.go and cache_test.go are totally rewrote, you can review the file directly.

siddontang · 2016-10-26T12:45:29Z

A huge PR, changed lines > 1024. Can you split it?

This commit is separated from #359, it remove some unnecessary code and wrap some code in clusterInfo.

* server/cache: wrap some methods in clusterInfo This commit is separated from #359, it remove some unnecessary code and wrap some code in clusterInfo.

huachaohuang · 2016-10-31T11:06:26Z

It's hard to split this PR any more, they depend on each other, and half of them are tests.
PTAL @overvenus @siddontang

siddontang · 2016-11-03T05:39:02Z

server/cache.go

-	oldRegion, ok := r.regions[region.GetId()]
-	if !ok {
-		log.Fatalf("updateRegion for none existed region in regions - %v", region)
+	if region.Leader == nil {


if no leader, do we need to clear up the leaders and followers cache for the region?

If the region exists, we will first remove the original region and add a new one, see setRegion.
addRegion and removeRegion are actually helper functions, only setRegion will be used outside.

siddontang · 2016-11-03T08:12:30Z

server/cache.go

+func (c *clusterInfo) getRegions() []*regionInfo {
+	c.RLock()
+	defer c.RUnlock()
+	return c.regions.getRegions()


seem it is still not thread-safe to use []*regionInfo?

getRegions will clone the regions.

If we have too many regions, this function may have bad performance, is it?

Yes, but seems no where use this function except for the get regions API.

siddontang · 2016-11-03T08:17:20Z

server/cache.go

-}
-
-func (r *regionsInfo) addRegionCount(region *metapb.Region) {
+	// Remove from leaders and followers.
 	for _, peer := range region.GetPeers() {


Can we meet following case:

1, add region a with peers 1,2,3,4
2, remove region a with peers 1,2,3

So the followers cache may have a uncleared peer.

When we remove a region, we will get the original region by id, which means that the removed region must be the same as the previous added region.

siddontang · 2016-11-03T11:48:44Z

LGTM

PTAL @overvenus @qiuyesuifeng

overvenus

LGTM

qiuyesuifeng · 2016-11-05T08:23:18Z

server/cache.go

@@ -14,14 +14,12 @@
 package server

 import (
-	"bytes"
-	"math/rand"
+	"log"


Please use github.com/ngaut/log.

qiuyesuifeng · 2016-11-05T08:23:41Z

server/cache.go

 	"sync"
 	"time"

-	"github.com/golang/protobuf/proto"
+	"github.com/gogo/protobuf/proto"


Why here use gogo proto?

Actually, goimports do that for me, seems they are mostly the same, and we do use gogo/protobuf in kvproto, I think it's OK.

regionsInfo will be rewrote later, we should not access it directly.

First, stores and regions are reorganized so that we can easily access the information we need. Second, the algorithm to handle region heartbeat is modified accordingly, to handle Split and Merge. It's hard to handle Split and Merge perfectly to keep the cache always reflecting the newest information about which region is serving which range. But we can keep the cache always updating according to the region heartbeat. Here's how we handle region heartbeat. First, if the region does not exist, it is added to the cache directly. Second, if the region exists but the heartbeat is stale, an error will be returned. Finally, if the region exists and the heartbeat is not stale, it is updated to the cache. Additionaly, every time a region is added or updated, all range overlap regions will be deleted from the search tree first, and then the region will be inserted. Note that regions deleted from the search tree can still be found by the region id.

siddontang changed the title ~~server/cache: reorganize the cache~~ [DNM] server/cache: reorganize the cache Oct 23, 2016

huachaohuang mentioned this pull request Oct 24, 2016

server: add kv #360

Merged

huachaohuang changed the base branch from huachaohuang/add-region-info to master October 25, 2016 08:46

huachaohuang force-pushed the huachaohuang/reorganize-cache branch from bc33d82 to bf80ab3 Compare October 25, 2016 08:49

huachaohuang changed the title ~~[DNM] server/cache: reorganize the cache~~ server/cache: reorganize the cache Oct 25, 2016

huachaohuang added a commit that referenced this pull request Oct 27, 2016

server/cache: wrap some methods in clusterInfo

43ec60a

This commit is separated from #359, it remove some unnecessary code and wrap some code in clusterInfo.

huachaohuang mentioned this pull request Oct 27, 2016

server/cache: wrap some methods in clusterInfo #365

Merged

huachaohuang force-pushed the huachaohuang/reorganize-cache branch from 0a88467 to 1347927 Compare October 27, 2016 05:10

huachaohuang changed the base branch from master to huachaohuang/add-stores-info October 27, 2016 05:10

huachaohuang changed the title ~~server/cache: reorganize the cache~~ [DNM] server/cache: reorganize the cache Oct 27, 2016

huachaohuang added a commit that referenced this pull request Oct 30, 2016

server/cache: wrap some methods in clusterInfo (#365)

91d4d3a

* server/cache: wrap some methods in clusterInfo This commit is separated from #359, it remove some unnecessary code and wrap some code in clusterInfo.

huachaohuang force-pushed the huachaohuang/add-stores-info branch from 3d21b4f to fd37e75 Compare October 30, 2016 04:04

huachaohuang mentioned this pull request Oct 31, 2016

[DNM] raftstore: let the remaining part inherit old region when split tikv/tikv#1187

Closed

huachaohuang force-pushed the huachaohuang/reorganize-cache branch from 1347927 to b2f0bd4 Compare October 31, 2016 10:40

huachaohuang changed the base branch from huachaohuang/add-stores-info to master October 31, 2016 10:41

huachaohuang changed the title ~~[DNM] server/cache: reorganize the cache~~ server/cache: reorganize the cache Oct 31, 2016

siddontang reviewed Nov 3, 2016

View reviewed changes

overvenus approved these changes Nov 4, 2016

View reviewed changes

qiuyesuifeng reviewed Nov 5, 2016

View reviewed changes

huachaohuang added 5 commits November 5, 2016 20:58

server/cache: hide regions in clusterInfo

f8581d3

regionsInfo will be rewrote later, we should not access it directly.

server/cache: tiny change

7cdbf91

server/cache: add tests

ceacc1b

server/cache: rebase

dacf5c2

huachaohuang added 2 commits November 5, 2016 20:59

server/cache: fix test

11c4fba

server/cache: address comment

5bc1700

huachaohuang force-pushed the huachaohuang/reorganize-cache branch from 64d6cbf to 5bc1700 Compare November 5, 2016 13:04

huachaohuang merged commit af843d1 into master Nov 6, 2016

huachaohuang deleted the huachaohuang/reorganize-cache branch December 7, 2016 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server/cache: reorganize the cache #359

server/cache: reorganize the cache #359

huachaohuang commented Oct 23, 2016

huachaohuang commented Oct 26, 2016

siddontang commented Oct 26, 2016

huachaohuang commented Oct 31, 2016

siddontang Nov 3, 2016

huachaohuang Nov 3, 2016

siddontang Nov 3, 2016

huachaohuang Nov 3, 2016

siddontang Nov 3, 2016

huachaohuang Nov 3, 2016

siddontang Nov 3, 2016

huachaohuang Nov 3, 2016

siddontang commented Nov 3, 2016

overvenus left a comment

qiuyesuifeng Nov 5, 2016

qiuyesuifeng Nov 5, 2016

huachaohuang Nov 5, 2016

server/cache: reorganize the cache #359

server/cache: reorganize the cache #359

Conversation

huachaohuang commented Oct 23, 2016

huachaohuang commented Oct 26, 2016

siddontang commented Oct 26, 2016

huachaohuang commented Oct 31, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Nov 3, 2016

overvenus left a 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