-
Notifications
You must be signed in to change notification settings - Fork 727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor cluster cache. #118
Conversation
c.RLock() | ||
defer c.RUnlock() | ||
|
||
return c.stores |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems that we may still meet concurrent problem when we use stores outer.
Lock does nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe needless, except for tests, if u call getStores to get stores, that will always be only readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get stores is readable, but add/remove store can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think it's not safe. Better to dump them into a []*StoreInfo
.
PTAL |
"github.com/pingcap/kvproto/pkg/pdpb" | ||
) | ||
|
||
type storeIDMap map[uint64]struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seem no used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it can be removed.
idx++ | ||
} | ||
|
||
return r.leaderRegions[randRegionID] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need clone()
for RegionInfo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is more safe.
LGTM |
} | ||
} | ||
|
||
r.leaderRegions[regionID] = &RegionInfo{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If version or conf version doesn't change, I think we can return directly, no need to update cache again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we should add this check later, maybe in next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so please add a TODO comment here.
Rest LGTM |
@siddontang PTAL |
* mcs, tso: fix potential inconsistency caused by non-atomic applying keyspace movement state change in the persistent store (tikv#6596) ref tikv#5895 fix potential inconsistency caused by non-atomic applying the state change in the persistent in the following cases: 1. Keyspace group split/merge 2. Keyspace movement across keyspace groups. Signed-off-by: Bin Shi <[email protected]> * client: fix keyspace update in `tsoSvcDiscovery` (tikv#6612) close tikv#6611 Signed-off-by: lhy1024 <[email protected]> --------- Signed-off-by: Bin Shi <[email protected]> Signed-off-by: lhy1024 <[email protected]> Co-authored-by: Bin Shi <[email protected]>
@siddontang PTAL