region cache: check if the pd returned regions covers the ranges #1377
Conversation
Signed-off-by: you06 <you1474600@gmail.com>
Signed-off-by: you06 <you1474600@gmail.com>
Signed-off-by: you06 <you1474600@gmail.com>
Signed-off-by: you06 <you1474600@gmail.com>
Signed-off-by: you06 <you1474600@gmail.com>
internal/locate/region_cache.go
Outdated
| backoffErr = errors.Errorf("PD returned no region, limit: %d", limit) | ||
| continue | ||
| } | ||
| if regionsHaveGap([]pd.KeyRange{{StartKey: startKey, EndKey: endKey}}, regionsInfo, limit) { | ||
| backoffErr = errors.Errorf("PD returned regions have gaps, limit: %d", limit) | ||
| continue |
There was a problem hiding this comment.
Is this an improvement for the original scan interface?
There was a problem hiding this comment.
Yes, this should be picked back to earlier versions, which fixes the gap correctness issue for tiflash coprocessor.
internal/locate/region_cache.go
Outdated
| // regionsHaveGap checks if the loaded regions can fully cover the key ranges. | ||
| // If there are any gaps between the regions, it returns true, then the requests might be retried. | ||
| // TODO: remove this function after PD client supports gap detection and handling it. | ||
| func regionsHaveGap(ranges []pd.KeyRange, regionsInfo []*pd.Region, limit int) bool { |
There was a problem hiding this comment.
The name seems not able to cover the actual usages which is more complex
Signed-off-by: you06 <you1474600@gmail.com>
| if len(checkKey) == 0 { | ||
| return false | ||
| } else if len(ranges[checkIdx].EndKey) == 0 { | ||
| return true |
There was a problem hiding this comment.
Is it possible the end key of the last region is also empty, should the true be returned in this case?
There was a problem hiding this comment.
This is the else branch of if len(checkKey) == 0, so here the checkKey is not empty, meaning that the returned regions didn't cover up to the very end.
Wait, checkKey looks never be assigned with an empty end_key as the function would immediately return at line 2257. The only possibility that it's empty must be an empty start_key which comes from the first range to check.
Then does it work when the ranges to check is ["", "b") and the regions is ["", "a")?
cc @you06
There was a problem hiding this comment.
Ah, sorry, it still looks fine.
But I suggest adding some test cases where there are only one requested range and only one region in the result, and the range is half unbounded (empty startKey + non-empty endKey and non-empty startKey + empty endKey).
There was a problem hiding this comment.
Half unbounded cases added.
internal/locate/region_cache.go
Outdated
| if r.Meta == nil { | ||
| return true | ||
| } | ||
| if len(r.Meta.StartKey) != 0 && bytes.Compare(r.Meta.StartKey, checkKey) > 0 { |
There was a problem hiding this comment.
The left half of the condition seems unnecessary?
| return false | ||
| } | ||
| checkKey = r.Meta.EndKey | ||
| for len(ranges[checkIdx].EndKey) > 0 && bytes.Compare(checkKey, ranges[checkIdx].EndKey) >= 0 { |
There was a problem hiding this comment.
Not very familiar with the usage of it. Is the ranges here guaranteed to be already sorted by their start key?
There was a problem hiding this comment.
Yes, the input ranges are sorted and cannot be overlapped.
MyonKeminta
changed the title
Signed-off-by: you06 <you1474600@gmail.com>
Signed-off-by: you06 <you1474600@gmail.com>
Signed-off-by: you06 <you1474600@gmail.com>
Signed-off-by: you06 <you1474600@gmail.com>
Signed-off-by: you06 <you1474600@gmail.com>
Signed-off-by: you06 <you1474600@gmail.com>
…) (#1379) * cherry pick #1377 to tidb-8.1 Signed-off-by: you06 <you1474600@gmail.com> * run ci test in tidb-8.1 Signed-off-by: you06 <you1474600@gmail.com> * lint Signed-off-by: you06 <you1474600@gmail.com> * rename function Signed-off-by: you06 <you1474600@gmail.com> * warn when no leader regions missing Signed-off-by: you06 <you1474600@gmail.com> --------- Signed-off-by: you06 <you1474600@gmail.com>
…) (#1380) * cherry pick #1377 to tidb-7.1 Signed-off-by: you06 <you1474600@gmail.com> * lint Signed-off-by: you06 <you1474600@gmail.com> * rename function Signed-off-by: you06 <you1474600@gmail.com> * warn when no leader regions missing Signed-off-by: you06 <you1474600@gmail.com> --------- Signed-off-by: you06 <you1474600@gmail.com>
…) (#1381) * cherry pick #1377 to tidb-7.5 Signed-off-by: you06 <you1474600@gmail.com> * run ci test in tidb-7.5 Signed-off-by: you06 <you1474600@gmail.com> * lint Signed-off-by: you06 <you1474600@gmail.com> * fix import Signed-off-by: you06 <you1474600@gmail.com> * rename function Signed-off-by: you06 <you1474600@gmail.com> * warn when no leader regions missing Signed-off-by: you06 <you1474600@gmail.com> --------- Signed-off-by: you06 <you1474600@gmail.com>
close #1376
Either
len(regionsInfo) == 0or gaps insideregionsInfoare valid cases, we should handle it. Unless we may suffer correctness issue if we read from regions with gap.This PR adds the checks if regions can fully cover the given ranges in
scanRegionsandbatchScanRegions. Since this is a rare case, I think we can just simply retry all the ranges if there are gaps.