kv: unify the BeginWith... function into one by Yisaer · Pull Request #23130 · pingcap/tidb

Yisaer · 2021-03-05T05:18:08Z

Signed-off-by: Song Gao disxiaofei@163.com

What problem does this PR solve?

Currently, we have serval BeginWithXXX functions in kv.Storage. In future, we are going to support time-bounded staleness transaction which may continue to add more BeginWithXXX functions.

What is changed and how it works?

This request unify all the BeginWithXXX functions into one and only leave 2 function like following:

	Begin() (Transaction, error)
	BeginWithOption(option TransactionOption) (Transaction, error)

Release note

No release note

Signed-off-by: Song Gao <disxiaofei@163.com>

Yisaer · 2021-03-05T05:18:54Z

/sig sql-infra

Yisaer · 2021-03-05T05:23:32Z

/cc @xhebox

Signed-off-by: Song Gao <disxiaofei@163.com>

Yisaer · 2021-03-05T05:52:39Z

/sig transaction

Yisaer · 2021-03-05T05:52:55Z

/cc @longfangsong

ti-chi-bot · 2021-03-05T05:52:56Z

@Yisaer: GitHub didn't allow me to request PR reviews from the following users: longfangsong.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @longfangsong

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

longfangsong · 2021-03-05T06:05:53Z

IMO Scope should be part of the TransactionOption.
I wonder would it be better if we make it something like:

type TransactionOption struct {
	StartTS *uint64
	PrevSec *uint64
	Scope *string
}

Then we could do something like (is it possible to have all these options set in the future?)

BeginWithOption(TransactionOption{}.SetStartTs(xxx).SetScope(yyy).SetPrevSec(zzz))

and implement BeginWithTxnScope with BeginWithOption

xhebox · 2021-03-05T06:13:42Z

is it possible to have all these options set in the future?

For now, it is exclusive. That is why there is an TransactionOptionType.

But I agree that only left one Begin(...Options) may be a good idea.

xhebox

LGTM. If you don't want to address the comment of @longfangsong , i.e. unify the api more aggressively. or want to address it in the next PR, I will give a lgtm.

ti-chi-bot · 2021-03-05T06:55:51Z

@xhebox: Please use /LGTM instead of LGTM when you want to approve the pull request by comment.
If you use the GitHub review feature, please approve the PR directly, the comment will not take effect in the GitHub review feature.
If you have any qustions please refer to lgtm command help or lgtm plugin design.

If you have approved this PR, please ignore this reply. This reply is being used as a temporary reply during the migration of the new bot and will be removed on April 1.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

longfangsong

/lgtm

We may discuss about more agressive refactoring later.

Signed-off-by: Song Gao <disxiaofei@163.com>

Yisaer · 2021-03-05T07:26:53Z

IMO Scope should be part of the TransactionOption.
I wonder would it be better if we make it something like:
type TransactionOption struct {
	StartTS *uint64
	PrevSec *uint64
	Scope *string
}
Then we could do something like (is it possible to have all these options set in the future?)
BeginWithOption(TransactionOption{}.SetStartTs(xxx).SetScope(yyy).SetPrevSec(zzz))
and implement BeginWithTxnScope with BeginWithOption

This advice LGTM. updated.

Signed-off-by: Song Gao <disxiaofei@163.com>

xhebox

Rest LGTM

xhebox · 2021-03-05T07:34:28Z

kv/kv.go

+	TxnScope string
+	StartTS  *uint64
+	PrevSec  *uint64


I don't think it is a good idea to use pointers for integers in go. It may bring another two escaped memory. Copying a slice of string or integers is free.

xhebox · 2021-03-05T07:37:53Z

session/session.go


 	// no need to get txn from txnFutureCh since txn should init with startTs
-	txn, err := s.store.BeginWithOption(s.GetSessionVars().CheckAndGetTxnScope(), kv.WithStartTSOption(startTS))
+	txn, err := s.store.BeginWithOption(kv.TransactionOption{}.SetTxnScope(s.GetSessionVars().CheckAndGetTxnScope()).SetStartTs(startTS))


Maybe give a NewTransactionOptionBuilder or just NewTransactionOption instead of constructing a struct by hand.

disksing · 2021-03-05T07:55:44Z

/run-all-tests

xhebox · 2021-03-05T08:13:05Z

/lgtm

ti-chi-bot · 2021-03-05T08:13:08Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

longfangsong
xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

Yisaer · 2021-03-05T08:27:50Z

/merge

ti-chi-bot · 2021-03-05T08:27:51Z

@Yisaer: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2021-03-05T08:27:53Z

@Yisaer: /merge is only allowed for the committers in list.

Details

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

xhebox · 2021-03-05T08:59:46Z

/merge

ti-chi-bot · 2021-03-05T08:59:47Z

@xhebox: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2021-03-05T08:59:49Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: e81b357

ti-chi-bot · 2021-03-05T08:59:56Z

@Yisaer: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

xhebox · 2021-03-05T09:42:05Z

/run-unit-test

unify begin

f2b9193

Signed-off-by: Song Gao <disxiaofei@163.com>

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 5, 2021

ti-chi-bot added the sig/sql-infra SIG: SQL Infra label Mar 5, 2021

ti-chi-bot requested a review from xhebox March 5, 2021 05:23

fix comment

ba02930

Signed-off-by: Song Gao <disxiaofei@163.com>

This comment has been minimized.

Sign in to view

ti-chi-bot added the sig/transaction SIG:Transaction label Mar 5, 2021

xhebox reviewed Mar 5, 2021

View reviewed changes

longfangsong approved these changes Mar 5, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 5, 2021

Yisaer added 2 commits March 5, 2021 15:13

address the comment

bba5fad

Signed-off-by: Song Gao <disxiaofei@163.com>

make fmt

3c74afc

Yisaer force-pushed the unify_begin branch from 3fe2cfa to 3c74afc Compare March 5, 2021 07:23

remove useless code

e81b357

Signed-off-by: Song Gao <disxiaofei@163.com>

xhebox reviewed Mar 5, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 5, 2021

This comment has been minimized.

Sign in to view

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 5, 2021

Merge branch 'master' into unify_begin

9e3789d

ti-chi-bot merged commit 707cb0a into pingcap:master Mar 5, 2021

longfangsong mentioned this pull request Mar 11, 2021

txn: Use TransactionOption in store/tikv module #23255

Merged

AndreMouche mentioned this pull request Mar 16, 2021

store/tikv:remove the usage of kv.TransactionOption #23352

Merged

Yisaer deleted the unify_begin branch March 26, 2021 09:00

Conversation

Yisaer commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Release note

Uh oh!

Yisaer commented Mar 5, 2021

Uh oh!

Yisaer commented Mar 5, 2021

Uh oh!

This comment has been minimized.

Yisaer commented Mar 5, 2021

Uh oh!

Yisaer commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

longfangsong commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xhebox commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xhebox left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

longfangsong left a comment

Choose a reason for hiding this comment

Uh oh!

Yisaer commented Mar 5, 2021

Uh oh!

xhebox left a comment

Choose a reason for hiding this comment

Uh oh!

xhebox Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

xhebox Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

disksing commented Mar 5, 2021

Uh oh!

xhebox commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

Yisaer commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

This comment has been minimized.

xhebox commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

ti-chi-bot commented Mar 5, 2021

Uh oh!

xhebox commented Mar 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Yisaer commented Mar 5, 2021 •

edited

Loading

longfangsong commented Mar 5, 2021 •

edited

Loading

xhebox commented Mar 5, 2021 •

edited

Loading