Skip to content

Commit

Permalink
Merge pull request #18709 from nvanbenschoten/nvanbenschoten/fixMetaC…
Browse files Browse the repository at this point in the history 
…ache

kv: fix RangeDescriptorCache eviction with meta2 splits
  • Loading branch information
@nvanbenschoten
nvanbenschoten authored Oct 3, 2017
2 parents 2f5c3f4 + 56563e6 commit 95713d3
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 41 deletions.
6 changes: 3 additions & 3 deletions pkg/kv/dist_sender_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,7 @@ func TestEvictCacheOnError(t *testing.T) {
if _, ok := ds.leaseHolderCache.Lookup(context.TODO(), 1); ok != !tc.shouldClearLeaseHolder {
t.Errorf("%d: lease holder cache eviction: shouldClearLeaseHolder=%t, but value is %t", i, tc.shouldClearLeaseHolder, ok)
}
if cachedDesc, err := ds.rangeCache.GetCachedRangeDescriptor(roachpb.RKey(key), false /* !inclusive */); err != nil {
if cachedDesc, err := ds.rangeCache.GetCachedRangeDescriptor(roachpb.RKey(key), false /* !inverted */); err != nil {
t.Error(err)
} else if cachedDesc == nil != tc.shouldClearReplica {
t.Errorf("%d: unexpected second replica lookup behaviour: wanted=%t", i, tc.shouldClearReplica)
Expand Down Expand Up @@ -1279,8 +1279,8 @@ func TestMultiRangeGapReverse(t *testing.T) {
if !reverse {
return key.Less(descs[i].EndKey)
}
// In reverse mode, the key is "inclusive". If we scan
// [a,z) in reverse mode, we'd look up key z.
// In reverse mode, the range boundary behavior is "inverted".
// If we scan [a,z) in reverse mode, we'd look up key z.
return !descs[i].EndKey.Less(key) // key <= EndKey
})
}
Expand Down
79 changes: 53 additions & 26 deletions pkg/kv/range_cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,8 @@ func (rdc *RangeDescriptorCache) lookupRangeDescriptorInternal(
defer rdc.rangeCache.Unlock()

// These need to be separate because we need to preserve the pointer to rs[0]
// so that the seenDesc logic works correctly in EvictCachedRangeDescriptor. An
// append could cause a copy, which would change the address of rs[0]. We insert
// so that the compare-and-evict logic works correctly in EvictCachedRangeDescriptor.
// An append could cause a copy, which would change the address of rs[0]. We insert
// the prefetched descriptors first to avoid any unintended overwriting. We then
// only insert the first desired descriptor, since any other descriptor in rs would
// overwrite rs[0]. Instead, these are handled with the evictToken.
Expand Down Expand Up @@ -425,28 +425,36 @@ func (rdc *RangeDescriptorCache) performRangeLookup(
return rdc.db.RangeLookup(ctx, metadataKey, desc, useReverseScan)
}

// EvictCachedRangeDescriptor will evict any cached range descriptors
// for the given key. It is intended that this method be called from a
// consumer of rangeDescriptorCache if the returned range descriptor is
// discovered to be stale.
// seenDesc should always be passed in and is used as the basis of a
// compare-and-evict (as pointers); if it is nil, eviction is unconditional
// but a warning will be logged.
// EvictCachedRangeDescriptor will evict any cached user-space and meta range
// descriptors for the given key. It is intended that this method be called from
// a consumer of rangeDescriptorCache through the EvictionToken abstraction if
// the returned range descriptor is discovered to be stale.
//
// seenDesc should always be passed in if available, and is used as the basis of
// a pointer-based compare-and-evict strategy. This means that if the cache does
// not contain the provided descriptor, no descriptor will be evicted. If
// seenDesc is nil, eviction is unconditional.
//
// `inverted` determines the behavior at the range boundary, similar to how it
// does in GetCachedRangeDescriptor.
func (rdc *RangeDescriptorCache) EvictCachedRangeDescriptor(
ctx context.Context, descKey roachpb.RKey, seenDesc *roachpb.RangeDescriptor, inclusive bool,
ctx context.Context, descKey roachpb.RKey, seenDesc *roachpb.RangeDescriptor, inverted bool,
) error {
rdc.rangeCache.Lock()
defer rdc.rangeCache.Unlock()
return rdc.evictCachedRangeDescriptorLocked(ctx, descKey, seenDesc, inclusive)
return rdc.evictCachedRangeDescriptorLocked(ctx, descKey, seenDesc, inverted)
}

// evictCachedRangeDescriptorLocked is like evictCachedRangeDescriptor, but it
// assumes that the caller holds a write lock on rdc.rangeCache.
func (rdc *RangeDescriptorCache) evictCachedRangeDescriptorLocked(
ctx context.Context, descKey roachpb.RKey, seenDesc *roachpb.RangeDescriptor, inclusive bool,
ctx context.Context, descKey roachpb.RKey, seenDesc *roachpb.RangeDescriptor, inverted bool,
) error {
cachedDesc, entry, err := rdc.getCachedRangeDescriptorLocked(descKey, inclusive)
if err != nil {
cachedDesc, entry, err := rdc.getCachedRangeDescriptorLocked(descKey, inverted)
if err != nil || cachedDesc == nil {
return err
}

// Note that we're doing a "compare-and-erase": If seenDesc is not nil,
// we want to clean the cache only if it equals the cached range
// descriptor as a pointer. If not, then likely some other caller
Expand All @@ -465,34 +473,53 @@ func (rdc *RangeDescriptorCache) evictCachedRangeDescriptorLocked(
// Retrieve the metadata range key for the next level of metadata, and
// evict that key as well. This loop ends after the meta1 range, which
// returns KeyMin as its metadata key.
descKey = keys.RangeMetaKey(descKey)
cachedDesc, entry, err = rdc.getCachedRangeDescriptorLocked(descKey, inclusive)
if err != nil {
return err
}
//
// Evicting 'meta(descKey)' instead of 'meta(cachedDesc.EndKey)' could be
// incorrect here because it could result in us evicting the wrong meta2
// descriptor. Imagine that descriptors are in the configuration:
//
// meta2 descs: [/meta2/a,/meta2/m), [/meta2/m,/meta2/z)
// user descs: [a,f), [f,k), [k,p), [p,u), [u,z)
//
// A lookup for the key 'l' would return the third user-space descriptor
// [k,p), after a continuation lookup. This descriptor is stored on the
// second meta2 range because its end key is p, which maps to /meta2/p.
// If we later want to evict this user-space descriptor, we also want to
// evict the meta2 descriptor for the range that contains it. However,
// if we naively evicted meta('l'), we'd end up evicting the first meta2
// descriptor. Instead, we want to evict meta('p'), since 'p' is the end
// key of the user-space descriptor.
//
// This is also why we pass inverted=false down below.
descKey = keys.RangeMetaKey(cachedDesc.EndKey)
// TODO(tschottdorf): write a test that verifies that the first descriptor
// can also be evicted. This is necessary since the initial range
// [KeyMin,KeyMax) may turn into [KeyMin, "something"), after which
// larger ranges don't fit into it any more.
if bytes.Equal(descKey, roachpb.RKeyMin) {
break
}

cachedDesc, entry, err = rdc.getCachedRangeDescriptorLocked(descKey, false /* !inverted */)
if err != nil || cachedDesc == nil {
return err
}
}
return nil
}

// GetCachedRangeDescriptor retrieves the descriptor of the range which contains
// the given key. It returns nil if the descriptor is not found in the cache.
//
// `inclusive` determines the behaviour at the range boundary: If set to true
// `inverted` determines the behavior at the range boundary: If set to true
// and `key` is the EndKey and StartKey of two adjacent ranges, the first range
// is returned instead of the second (which technically contains the given key).
func (rdc *RangeDescriptorCache) GetCachedRangeDescriptor(
key roachpb.RKey, inclusive bool,
key roachpb.RKey, inverted bool,
) (*roachpb.RangeDescriptor, error) {
rdc.rangeCache.RLock()
defer rdc.rangeCache.RUnlock()
desc, _, err := rdc.getCachedRangeDescriptorLocked(key, inclusive)
desc, _, err := rdc.getCachedRangeDescriptorLocked(key, inverted)
return desc, err
}

Expand All @@ -502,12 +529,12 @@ func (rdc *RangeDescriptorCache) GetCachedRangeDescriptor(
// In addition to GetCachedRangeDescriptor, it also returns an internal cache
// Entry that can be used for descriptor eviction.
func (rdc *RangeDescriptorCache) getCachedRangeDescriptorLocked(
key roachpb.RKey, inclusive bool,
key roachpb.RKey, inverted bool,
) (*roachpb.RangeDescriptor, *cache.Entry, error) {
// The cache is indexed using the end-key of the range, but the
// end-key is non-inclusive by default.
// end-key is non-inverted by default.
var metaKey roachpb.RKey
if !inclusive {
if !inverted {
metaKey = keys.RangeMetaKey(key.Next())
} else {
metaKey = keys.RangeMetaKey(key)
Expand All @@ -520,7 +547,7 @@ func (rdc *RangeDescriptorCache) getCachedRangeDescriptorLocked(
desc := entry.Value.(*roachpb.RangeDescriptor)

containsFn := (*roachpb.RangeDescriptor).ContainsKey
if inclusive {
if inverted {
containsFn = (*roachpb.RangeDescriptor).ContainsKeyInverted
}

Expand Down
88 changes: 76 additions & 12 deletions pkg/kv/range_cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -837,20 +837,19 @@ func TestRangeCacheClearOverlappingMeta(t *testing.T) {
defer leaktest.AfterTest(t)()
ctx := context.TODO()

firstDesc := &roachpb.RangeDescriptor{
firstDesc := roachpb.RangeDescriptor{
StartKey: roachpb.RKeyMin,
EndKey: roachpb.RKey("zzz"),
}
restDesc := &roachpb.RangeDescriptor{
StartKey: firstDesc.StartKey,
restDesc := roachpb.RangeDescriptor{
StartKey: firstDesc.EndKey,
EndKey: roachpb.RKeyMax,
}

cache := NewRangeDescriptorCache(nil, staticSize(2<<10))
cache.rangeCache.cache.Add(rangeCacheKey(keys.RangeMetaKey(firstDesc.EndKey)),
firstDesc)
cache.rangeCache.cache.Add(rangeCacheKey(keys.RangeMetaKey(restDesc.EndKey)),
restDesc)
if err := cache.InsertRangeDescriptors(ctx, firstDesc, restDesc); err != nil {
t.Fatal(err)
}

// Add new range, corresponding to splitting the first range at a meta key.
metaSplitDesc := &roachpb.RangeDescriptor{
Expand All @@ -869,9 +868,75 @@ func TestRangeCacheClearOverlappingMeta(t *testing.T) {
}()
}

// TestGetCachedRangeDescriptorInclusive verifies the correctness of the result
// that is returned by getCachedRangeDescriptor with inclusive=true.
func TestGetCachedRangeDescriptorInclusive(t *testing.T) {
// TestRangeCacheEvictMetaDescriptors tests that regardless of the eviction key
// provided to EvictCachedRangeDescriptor, the meta ranges that store the key's
// descriptor and its meta descriptors are properly evicted. This is related to
// #18032.
func TestRangeCacheEvictMetaDescriptors(t *testing.T) {
defer leaktest.AfterTest(t)()
ctx := context.TODO()

meta1Desc := roachpb.RangeDescriptor{
StartKey: roachpb.RKeyMin,
EndKey: keys.MustAddr(keys.Meta2Prefix),
}
meta2LeftDesc := roachpb.RangeDescriptor{
StartKey: meta1Desc.EndKey,
EndKey: keys.RangeMetaKey(roachpb.RKey("m")),
}
meta2RightDesc := roachpb.RangeDescriptor{
StartKey: meta2LeftDesc.EndKey,
EndKey: keys.MustAddr(keys.Meta2KeyMax.Next()),
}
restDesc := roachpb.RangeDescriptor{
StartKey: meta2RightDesc.EndKey,
EndKey: roachpb.RKeyMax,
}

testCases := []struct {
evictKey roachpb.RKey
}{
{evictKey: roachpb.RKey("a")},
{evictKey: roachpb.RKey("m")},
{evictKey: roachpb.RKey("z")},
}
for _, tc := range testCases {
t.Run(tc.evictKey.String(), func(t *testing.T) {
if !restDesc.ContainsKey(tc.evictKey) {
t.Fatalf("restDesc must contain evictKey, found %v", tc.evictKey)
}

cache := NewRangeDescriptorCache(nil, staticSize(2<<10))
err := cache.InsertRangeDescriptors(ctx, meta1Desc, meta2LeftDesc, meta2RightDesc, restDesc)
if err != nil {
t.Fatal(err)
}

// Evict the user-space descriptor. This should result in the metaRightDesc
// being evicted as well, because that is where restDesc is stored. This
// is true even if meta(tc.evictKey) addresses into metaLeftDesc. In addition,
// the meta1Desc will be evicted, leaving only the meta2RightDesc.
err = cache.EvictCachedRangeDescriptor(ctx, tc.evictKey, nil, false)
if err != nil {
t.Fatal(err)
}

exp := []roachpb.RangeDescriptor{meta2LeftDesc}
found := []roachpb.RangeDescriptor{}
cache.rangeCache.cache.Do(func(k, v interface{}) bool {
found = append(found, *v.(*roachpb.RangeDescriptor))
return false
})
if !reflect.DeepEqual(exp, found) {
t.Errorf("expected remaining descriptors %v, found %v", exp, found)
}
})
}
}

// TestGetCachedRangeDescriptorInverted verifies the correctness of the result
// that is returned by getCachedRangeDescriptor with inverted=true.
func TestGetCachedRangeDescriptorInverted(t *testing.T) {
defer leaktest.AfterTest(t)()

testData := []*roachpb.RangeDescriptor{
Expand Down Expand Up @@ -925,7 +990,7 @@ func TestGetCachedRangeDescriptorInclusive(t *testing.T) {
for _, test := range testCases {
cache.rangeCache.RLock()
targetRange, entry, err := cache.getCachedRangeDescriptorLocked(
test.queryKey, true /* inclusive */)
test.queryKey, true /* inverted */)
cache.rangeCache.RUnlock()
if err != nil {
t.Error(err)
Expand All @@ -941,5 +1006,4 @@ func TestGetCachedRangeDescriptorInclusive(t *testing.T) {
t.Fatalf("expect cache key %v, actual get %v", test.cacheKey, cacheKey)
}
}

}

0 comments on commit 95713d3

Please sign in to comment.