Skip to content
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.

Sign up for GitHub

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

Jump to bottom

store/tikv: support fallback from async commit (take 2) #21531

Merged
merged 29 commits into from
Dec 10, 2020
Merged

store/tikv: support fallback from async commit (take 2) #21531

merged 29 commits into from
Dec 10, 2020

Conversation

sticnarf
Copy link
Contributor

@sticnarf sticnarf commented Dec 7, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: tikv/sig-transaction#64

What is changed and how it works?

Proposal: tikv/sig-transaction#64 (comment)

If async-commit or 1PC prewrite receives min_commit_ts = 0, it will automatically use sync 2PC to commit.

When resolving locks, it firstly judges the transaction by the information inside of the primary lock. If use_async_commit is true for the primary lock, it will do CheckSecondaryLocks. If CheckSecondaryLocks sees a lock whose use_async_commit is false, we retry CheckTxnStatus with forceSyncCommit set to true. Then, we are treating this transaction as a sync 2PC transaction.

Related changes

Check List

Tests

  • Unit test

Release note

  • Part of the async commit feature.

@sticnarf
implement new resolver
2543b3c 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
update proto
b3899e8 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
disable async commit when 1pc fallback
1ff0410 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
update kvproto
a289c9c 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
fix compile
2af8f9d 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
add resolve lock tests
006fb08 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
update kvproto
6f86958 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
rename
5bf99bb 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf added the sig/transaction SIG:Transaction label Dec 7, 2020
@sticnarf sticnarf mentioned this pull request Dec 7, 2020
Merged
1 task
@sticnarf
Copy link
Contributor Author

sticnarf commented Dec 8, 2020

/run-all-tests tikv=pr/9196

@sticnarf
Copy link
Contributor Author

sticnarf commented Dec 8, 2020

@nrc @youjiali1995 @MyonKeminta Could you take a look at this PR and all its dependencies?

@sticnarf
update kvproto
b569fff 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
Merge remote-tracking branch 'upstream/master' into fallback-v2
54663b9
@sticnarf
Merge remote-tracking branch 'upstream/master' into fallback-v2
bee7dfd
@sticnarf
support commit with schema change
df070a5 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf
update unistore
cacb8c6 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
youjiali1995
youjiali1995 reviewed Dec 8, 2020
View reviewed changes
Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 8, 2020
@sticnarf
Merge remote-tracking branch 'upstream/master' into fallback-v2
df070f3 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
MyonKeminta
MyonKeminta reviewed Dec 9, 2020
View reviewed changes
store/tikv/prewrite.go
@@ -162,6 +168,7 @@ func (action actionPrewrite) handleSingleBatch(c *twoPhaseCommitter, bo *Backoff
zap.Uint64("startTS", c.startTS))
tikvOnePCTxnCounterFallback.Inc()
c.setOnePC(false)
c.setAsyncCommit(false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it's here, instead of in the if clause at L190?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using 1pc, useAsyncCommit may be true. This field should be set false when fallback from 1pc.
Otherwise, we will get into the wrong branch in https://github.com/pingcap/tidb/blob/0eb8ff97bc/store/tikv/2pc.go#L1162 and https://github.com/pingcap/tidb/blob/0eb8ff97bc/store/tikv/2pc.go#L1235

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry. I just notice that L196 has already unset async commit for non-1pc case. But for 1pc case I recommend that also check min_commit_ts field to be more strict. There's current no case that 1pc may fallback to async commit though.

MyonKeminta
MyonKeminta reviewed Dec 9, 2020
View reviewed changes
Copy link
Contributor

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

store/tikv/prewrite.go
@@ -162,6 +168,7 @@ func (action actionPrewrite) handleSingleBatch(c *twoPhaseCommitter, bo *Backoff
zap.Uint64("startTS", c.startTS))
tikvOnePCTxnCounterFallback.Inc()
c.setOnePC(false)
c.setAsyncCommit(false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry. I just notice that L196 has already unset async commit for non-1pc case. But for 1pc case I recommend that also check min_commit_ts field to be more strict. There's current no case that 1pc may fallback to async commit though.

@sticnarf
return error when minCommitTs is not zero during 1pc fallback
7ed4751 
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
MyonKeminta
MyonKeminta previously approved these changes Dec 9, 2020
View reviewed changes
@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 9, 2020
ti-srebot
ti-srebot previously approved these changes Dec 9, 2020
View reviewed changes
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 9, 2020
@sticnarf sticnarf added the status/can-merge Indicates a PR has been approved by a committer. label Dec 9, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21591
  • 21570
  • 21588
  • 21495
  • 19787

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@sticnarf merge failed.

@sticnarf sticnarf dismissed stale reviews from ti-srebot and MyonKeminta via f9ce470 December 10, 2020 02:48
@sticnarf
Copy link
Contributor Author

/run-all-tests

@sticnarf
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21627

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit bc41e47 into pingcap:master Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youjiali1995 youjiali1995 youjiali1995 left review comments

@MyonKeminta MyonKeminta MyonKeminta approved these changes

@ti-srebot ti-srebot ti-srebot left review comments

Assignees
No one assigned
Labels
sig/transaction SIG:Transaction status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sticnarf @ti-srebot @MyonKeminta @youjiali1995