store-tikv: drop invalid cached region #4506

atmzhou · 2017-09-12T09:07:44Z

Some cached region may be out-of-date for they are long not used.
To avoid use these regions, we allocate each cached region a TTL, which records when the region may be out-of-date.
When an out-of-dated region is identified, we remove the region out of the region cache.

Fix #2511
Fix #4498

coocood · 2017-09-12T09:15:36Z

@atmzhou
I think adding an accessTime uint64 field in Region is simpler.
Multiple threads may update the access time concurrently,
we can update it with atomic operations to avoid race.

coocood · 2017-09-12T09:16:21Z

@atmzhou
Please update the title of the PR.

atmzhou · 2017-09-12T09:21:07Z

@coocood
Do you think it is necessary to replace the mutex by atomic operation?

atmzhou · 2017-09-12T09:26:27Z

@disksing @AndreMouche @tiancaiamao
Could you please review my code at convenience?
Thanks~

coocood · 2017-09-12T09:27:27Z

@atmzhou
We can't replace the mutex by atomic operation.
RegionCache uses RWLock which allows concurrent reading.
But reading updates the access time, which is a write operation, so we need atomic operation to avoid race.

atmzhou · 2017-09-12T09:33:47Z

@coocood
So, what is your conclusion? Should we use the RWLock or atomic operations？
By the way, I think that the data race is rare and a little data race does not matter here.

coocood · 2017-09-12T09:44:10Z

@atmzhou
We use both RWLock and atomic operation, RWLock for the RegionCache, atomic operation for the Region.
Data race is strictly not allowed in TiDB. The CI fails if any data race is detected.

atmzhou · 2017-09-13T00:28:29Z

@coocood
OK, I will eliminate any potential data race here.
By the way, in fact, this data race cannot be detected because it does not affect correctness. It just removes some region from the cache falsely or accesses an out-of-dated region, which will also be handled.

coocood · 2017-09-13T02:42:27Z

@atmzhou
I will be detected when two goroutines access the same variable concurrently and at least one of the accesses is a write.

See https://golang.org/doc/articles/race_detector.html

tiancaiamao · 2017-09-13T10:52:52Z

store/tikv/region_cache.go

 		sync.RWMutex
 		regions map[RegionVerID]*Region
 		sorted  *llrb.LLRB
+		ttls    map[RegionVerID]time.Time


How about move the ttl field to Region, so no extra map need to be maintained.

Region should not have a field ttl.
If we want to save a map structure, we can merge the two maps as
regions map[RegionVerID]<*Region, time.Time>.
However, this may incur updating a lot of codes.
So, I prefer keeping the two maps separately.

@atmzhou
Then how about accessTime, Region can have an accessTime field.

@coocood @tiancaiamao
I have make a structure named CachedRegion as follows:
type CachedRegion struct {
region Region
ttl int64
}
Accordingly, each RegionCache will have a map map[RegionVerID]*CachedRegion.
In this way, we save a map structure.
I think this should conform to your thought.

@atmzhou
ttl is changing over time, we can't store a ttlvalue in field.

coocood · 2017-09-18T07:30:38Z

store/tikv/region_cache.go

+// CachedRegion encapsulates {Region, TTL}
+type CachedRegion struct {
+	region	*Region
+	ttl		int64


ttl is a duration, not a timestamp, why not name it lastAccess directly?

Ok, I will change it. Thanks~

coocood · 2017-09-18T07:32:50Z

store/tikv/region_cache.go

+
+// CachedRegion encapsulates {Region, TTL}
+type CachedRegion struct {
+	region	*Region


Why not rename Region to CachedRegion directly and add the new field there?

Regions may be used to partition requests. For this kind of tasks, ttl is of no use.
In fact, ttl is only used for cache.
So, I prefer defining a new structure CachedRegion here.

The Region is a cached region with or without access time.
Use different type just to add the access time makes confusion.

coocood · 2017-09-18T07:37:00Z

store/tikv/region_cache.go

+	// For example, T1 find ttl valid but before T1 freshes the ttl, T2 find ttl invalid
+	// In this case, T2 will invoke this function but T1 may have freshed the ttl
+	// Then, T2 should not drop the cached region
+	ttl := atomic.LoadInt64(&cachedregion.ttl) // we may not need an atomic load, I think a simple load is enough?


I think we don't need to check the access time again, just delete it.

Indeed, both rechecking and directly deleting are correct.
Directly deleting may simplify the code.
However, I think it leads to strange cases. Suppose that there are two visits on the same cached region. If the first visit determines to refresh the ttl of the region, when it conducts this action, it may find that the region was removed because of out-of-date. This is strange.

The first reader found the region is valid, then he will just refersh the region, he won't lookup the map for the region again, so he never find that region was removed.

Yes，You are right.
Either deleting or rechecking is correct.
The choice depends on whether we want the execution is equivalent to the actual happening order of the two operations.
By rechecking, I choose that the execution should be equivalent to the actual happening order of the two operations.
This may be more strict than directly deleting.
I think the two possible choices are OK.

coocood · 2017-09-18T08:05:08Z

store/tikv/region_cache.go

+func (c *RegionCache) GetCachedRegion(id RegionVerID) *Region {
+	for {
+		c.mu.RLock()
+		cachedregion, ok := c.mu.regions[id]


We can just RUnlock here.
The lock only protects the regions map.

Yes, I will change the code. Thanks~

disksing · 2017-09-19T02:06:02Z

store/tikv/region_cache.go

+	lastAccessTime := time.Unix(lastAccess, 0)
+	if time.Since(lastAccessTime) < rcDefaultRegionCacheTTL {
+		return false
+	}


Seems we can extract L94-L98 into a method of CachedRegion.

Thanks, I will revise this.

disksing · 2017-09-19T10:59:21Z

store/tikv/region_cache.go

@@ -26,12 +28,23 @@ import (
 	goctx "golang.org/x/net/context"
 )

+const (
+	rcDefaultRegionCacheTTL = time.Minute * 2


Maybe a longer duration is more suitable. 10 minute for instance.

Ok, Thanks~

disksing · 2017-09-19T11:01:00Z

store/tikv/region_cache.go

+	if time.Since(lastAccessTime) < rcDefaultRegionCacheTTL {
+		return true
+	}
+	return false


Just return time.Since(lastAccessTime) < rcDefaultRegionCacheTTL.

OK, Thanks~

disksing · 2017-09-19T11:01:41Z

store/tikv/region_cache.go

@@ -66,17 +79,61 @@ func (c *RPCContext) GetStoreID() uint64 {
 	return 0
 }

+func (c *CachedRegion) isValid() bool {
+	lastAccess := atomic.LoadInt64(&c.lastAccess) // we may not need an atomic load, I think a simple load is enough?


We need to use atomic, or the race detector will complain. Please remove the comment.

OK, Thanks~
By the way, in fact, we do not need to use atomic.

tiancaiamao · 2017-09-20T02:33:57Z

store/tikv/region_cache.go

+
+// CachedRegion encapsulates {Region, TTL}
+type CachedRegion struct {
+	region     *Region


How about region Region ?

tiancaiamao · 2017-09-20T02:35:56Z

store/tikv/region_cache.go

+// CachedRegion encapsulates {Region, TTL}
+type CachedRegion struct {
+	region     *Region
+	lastAccess int64


lastAccess time.Time ?

we use int64 to enable atomic operation

tiancaiamao · 2017-09-20T02:41:42Z

store/tikv/region_cache.go

+	// For example, T1 find ttl valid but before T1 freshes the ttl, T2 find ttl invalid
+	// In this case, T2 will invoke this function but T1 may have freshed the ttl
+	// Then, T2 should not drop the cached region
+	if cachedregion.isValid() {


It's under lock, why there are concurrent threads may fresh it ?

Because concurrent threads use atomic operations to fresh it.
In this way, we save write lock.

disksing · 2017-09-21T11:06:33Z

LGTM.

coocood · 2017-09-21T11:33:41Z

store/tikv/region_cache.go

@@ -138,26 +168,24 @@ func (c *RegionCache) LocateKey(bo *Backoffer, key []byte) (*KeyLocation, error)

 // LocateRegionByID searches for the region with ID
 func (c *RegionCache) LocateRegionByID(bo *Backoffer, regionID uint64) (*KeyLocation, error) {
-	c.mu.RLock()
-	if r := c.getRegionByIDFromCache(regionID); r != nil {
+	r := c.getRegionByIDFromCache(regionID)


@disksing Should we use GetCachedRegion here?
This method is only used by HTTP status server.

They have different parameters. BTW, I think we need lock here. @atmzhou

@disksing I remember that you said this function is only used for test. So I did not add lock in getRegionByIDFromCache.
Do you think we should add lock? If yes, I will add.

Yes. We don't want debug API have data race too, especially it may affect online service.

disksing · 2017-09-25T02:38:29Z

PTAL @coocood

coocood

LGTM

atmzhou requested review from AndreMouche, tiancaiamao and disksing September 12, 2017 09:07

atmzhou changed the title ~~Ningnanzhou/dropinvalidregioncache~~ Ningnanzhou/DropInvalidCachedRegion Sep 12, 2017

atmzhou changed the title ~~Ningnanzhou/DropInvalidCachedRegion~~ store-tikv/DropInvalidCachedRegion Sep 12, 2017

atmzhou changed the title ~~store-tikv/DropInvalidCachedRegion~~ store-tikv: drop invalid cached region Sep 12, 2017

atmzhou closed this Sep 12, 2017

atmzhou reopened this Sep 12, 2017

tiancaiamao reviewed Sep 13, 2017

View reviewed changes

revise an issue of double lock

f7305aa

atmzhou force-pushed the ningnanzhou/dropinvalidregioncache branch from d01a2d0 to f7305aa Compare September 18, 2017 07:18

atmzhou added 3 commits September 18, 2017 15:20

remove unused log.Error

bdf60c9

remove unused log.Error in test file

80f17c5

remove unused import in test file

ade7e00

coocood reviewed Sep 18, 2017

View reviewed changes

format the code to pass test

079dbfc

coocood reviewed Sep 18, 2017

View reviewed changes

atmzhou added 2 commits September 18, 2017 16:43

revise according to comments from coocood

21d8e4e

reduce redundant code according to menglong's comment

cfccf98

atmzhou added 6 commits September 18, 2017 17:59

Merge branch 'master' into ningnanzhou/dropinvalidregioncache

7f1a6bf

golint failed after I merge with the latest master

1f73620

revise to happy golint

58a4226

revise according to menglong's comment

625dc49

directly deleting

153606b

resolve disksing's comments

cf482f0

disksing reviewed Sep 19, 2017

View reviewed changes

atmzhou force-pushed the ningnanzhou/dropinvalidregioncache branch from 5ed707b to 7f1a6bf Compare September 19, 2017 02:38

disksing reviewed Sep 19, 2017

View reviewed changes

tiancaiamao reviewed Sep 20, 2017

View reviewed changes

coocood reviewed Sep 21, 2017

View reviewed changes

atmzhou force-pushed the ningnanzhou/dropinvalidregioncache branch from cf8a5b8 to cf482f0 Compare September 22, 2017 09:27

Merge branch 'master' into ningnanzhou/dropinvalidregioncache

13a527d

coocood approved these changes Sep 25, 2017

View reviewed changes

Merge branch 'master' into ningnanzhou/dropinvalidregioncache

8160183

atmzhou merged commit b9c3bc4 into master Sep 25, 2017

atmzhou deleted the ningnanzhou/dropinvalidregioncache branch September 25, 2017 04:59

disksing mentioned this pull request Dec 14, 2017

store/tikv: no need to manually drop region when RPC call fails. #5402

Closed

lysu mentioned this pull request Aug 29, 2019

tikv: cleanup outdate regions's memory from region-cache in 2.1 #11931

Merged

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

store-tikv: drop invalid cached region #4506

store-tikv: drop invalid cached region #4506

Conversation

atmzhou commented Sep 12, 2017

coocood commented Sep 12, 2017 • edited Loading

coocood commented Sep 12, 2017

atmzhou commented Sep 12, 2017

atmzhou commented Sep 12, 2017

coocood commented Sep 12, 2017

atmzhou commented Sep 12, 2017 • edited Loading

coocood commented Sep 12, 2017

atmzhou commented Sep 13, 2017

coocood commented Sep 13, 2017

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

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

coocood Sep 18, 2017 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disksing commented Sep 21, 2017

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

disksing commented Sep 25, 2017

coocood left a comment

Choose a reason for hiding this comment

coocood commented Sep 12, 2017 •

edited

Loading

atmzhou commented Sep 12, 2017 •

edited

Loading

coocood Sep 18, 2017 •

edited

Loading