How to break the dependency to tidb/kv.TransactionOption for store/tikv

#1

Hi here!

I am trying to get involved with the big effort Provide standalone Go client based on the one embedded as store/tikv inside pingcap/tidb repo.

Running the attached command gives remaining dependencies from store/tikv to tidb.

$ grep -r "github.com/pingcap/tidb" store/tikv | grep -v "github.com/pingcap/tidb/store/tikv" | grep -v "failpoint" | awk '{if(NF==3){print $3,$1}else{print $2,$1}}' | grep -v "store/tikv/tests" | sort -u

"github.com/pingcap/tidb/kv" store/tikv/extract_start_ts_test.go:
"github.com/pingcap/tidb/kv" store/tikv/kv.go:
"github.com/pingcap/tidb/kv" store/tikv/txn.go:
"github.com/pingcap/tidb/store/mockstore/unistore" store/tikv/extract_start_ts_test.go:
// store/tikv/region_request.go:
It store/tikv/util/testleak/check-leaktest.sh:#

while which is reported in store/tikv/kv.go and store/tikv/txn.go is tidb/kv.TransactionOption. I have no idea how to break the dependency to it.

I post the question here instead of the issue because the problem is not how to break a code dependency but how TiDB uses TransactionOption to interact with tikv (client) for implementing transaction.

With the answer of this question it should be clear what to do to the codebase, as well as the answer should be useful for those who are interested with this part beyond a code refactor only.

#2

AFAIK, TransactionOption is used for identify which timestamp we should use as start_ts to support stale read, cc the function extractStartTs.

IMO it is this struct should be in the store/tikv, because it’s deeply connected with things like timestamp-getting and should be able to work without TiDB (though TiDB is the only one who want this feature now).

I’m already working on breaking the dependency on this branch

PS. I don’t like the name TransactionOption. It’s too wide and might be confused with many other things (eg. things like isPessimistic, enableAsyncCommit should be “Option” too). Maybe TimestampGettingPolicy or something alike would be better.

Update: A PR has already been sent.

1 Like
#3

Good job!

With offline discussion with the driver @disking, the best practice for breaking dependencies is,

  1. Move code under store/tikv, and cleanup dependency graph.
  2. If then there are multiple dependencies from tidb to tikv client, it is unreasonable. In order to avoid pollute dependencies to tikv specific concept everywhere inside tidb, we create a similar adapt concept under package kv . You can take this pull request as an example.
  3. The new concept is not coupled with tikv specific concept, and we handle the mapping in store/driver.
1 Like