-
Notifications
You must be signed in to change notification settings - Fork 227
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
feat: Broadcast min_commit_ts for pipelined transactions #1458
feat: Broadcast min_commit_ts for pipelined transactions #1458
Conversation
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
1789ae4
to
ad0302e
Compare
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.
BTW, this is not related to this PR, can you change the RLock/RUnlock
to Lock/Unlock
here?
I have a question about the future plan, have you considered apply this broadcast also for slow or large 2pc transactions?
…-ts-for-large-txn
Co-authored-by: you06 <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
Done.
It's possible. But I think we need more empirical data before implementing that. It's unclear now how this optimization would benefit normal large transactions. |
Co-authored-by: crazycs <[email protected]>
Signed-off-by: ekexium <[email protected]>
Co-authored-by: crazycs <[email protected]>
Signed-off-by: ekexium <[email protected]>
…lient-go into feat-resolved-ts-for-large-txn
internal/locate/region_cache.go
Outdated
if store == nil || store.GetState() == metapb.StoreState_Tombstone { | ||
s.setResolveState(tombstone) | ||
continue | ||
} | ||
addr := store.GetAddress() | ||
if addr == "" { | ||
continue |
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.
Better to explain in the comments when the pdClient.GetAllStores(ctx)
may return a store list containg nil
store and emptry address
and how should they be treated. For example, dose it mean the returned store list
is invalid and pdClient.GetAllStores(ctx)
should be retried when there invalid element in the list?
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 believe that GetAllStores
is supposed to return only stores in Up and Offline states. This additional check serves as a defensive measure and ensures consistency with the store resolve procedure
Signed-off-by: ekexium <[email protected]>
wg.Wait() | ||
} | ||
|
||
if err := txn.spawnWithStorePool(broadcastFunc); err != nil { |
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.
Since we've already used a goroutine pool, how about spawning broadcast tasks like the following:
stores := store.GetRegionCache().GetStoresByType(tikvrpc.TiKV)
rateLimit := make(chan struct{}, min(broadcastMaxConcurrency, len(stores)))
for _, s := range stores {
rateLimit <- struct{}{}
target := s
err := txn.spawnWithStorePool(func() {
defer func() {
<-rateLimit
}()
// send request to target
})
if err != nil {
<-rateLimit
}
}
Signed-off-by: ekexium <[email protected]>
Signed-off-by: ekexium <[email protected]>
afbcb64
to
4aada7a
Compare
…-ts-for-large-txn
3e9a4ec
to
012425a
Compare
Signed-off-by: ekexium <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, zyguan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Ref: tikv/tikv#17459
Prerequires pingcap/kvproto#1265
Changes