-
Notifications
You must be signed in to change notification settings - Fork 131
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
pump/: Accelerat transaction status queries throught GetMvccByEncodeKey #632
Conversation
pump/server.go
Outdated
|
||
if len(binlog.PrewriteKey) > 0 { | ||
tikvStorage := s.tiStore.(tikv.Storage) | ||
healper := storage.Helper{ |
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.
why not one Helper
instance?
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.
no performance cost and only use here so I think better just construct here, don't make Server struct complicate
pump/server.go
Outdated
RegionCache: tikvStorage.GetRegionCache(), | ||
} | ||
|
||
resp, err := healper.GetMvccByEncodedKey(binlog.PrewriteKey) |
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.
we can extract one function queryTransactionStatus
, then just call it. the implementation output too many unless and old mvcc records
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.
for debug just keep all info?
Good Job! |
should we put |
} | ||
kvResp, err := h.Store.SendReq(tikv.NewBackoffer(context.Background(), 500), tikvReq, keyLocation.Region, time.Minute) | ||
if err != nil { | ||
log.Info("get MVCC by encoded key failed", |
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.
log.Info("get MVCC by encoded key failed", | |
log.Error("get MVCC by encoded key failed", |
} | ||
|
||
// GetMvccByEncodedKey get the MVCC value by the specific encoded key. | ||
func (h *Helper) GetMvccByEncodedKey(encodedKey kv.Key) (*kvrpcpb.MvccGetByKeyResponse, error) { |
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.
How about just define a function GetMVCCByEncodedKey(tikv.Storage, kv.Key)
instead? It seems like we have no other use for the Helper
struct here.
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.
// Copy from https://github.com/pingcap/tidb/blob/71def9c7263432c0dfa6a5960f6db824775177c9/store/helper/helper.go#L47
// we can use it directly if we upgrade to the latest version of TiDB dependency.
add comment in helper.go
and we may add other myself if not use TiDB code directly.
/run-all-tests |
/run-all-tests |
1 similar comment
/run-all-tests |
LGTM |
/run-all-tests |
/run-all-tests |
/run-unit-test |
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.
LGTM
/run-all-tests |
What problem does this PR solve?
Avoid some case that we wait 10 minutes after the transaction and call
GetTxnStatus
which send the CleanupRequest that will get the commit ts of transaction or rollback the transaction.What is changed and how it works?
Query txn info by GetMvccByEncodeKey.
we can know if the txn if committed or rollback as quick as possible.
If we can't get the status we will fallback to the case call GetTxnStatus after 10 minute of the transaction.
Check List
Tests
log of the pump with this pr
.74 is the version with this pr
Code changes
Side effects
Related changes