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

txn: fix some logs and assumptions are inaccurate when the async commit protocol is used #24140

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

txn: fix some logs and assumptions are inaccurate when the async commit protocol is used #24140

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
483e408
fix log
zhaoxugang Apr 19, 2021
bf478db
Merge branch 'master-upstream' into first-transaction
zhaoxugang Apr 19, 2021
f37e759
fix log
zhaoxugang Apr 21, 2021
27e53b5
fix log
zhaoxugang Apr 24, 2021
fc44c3c
fix log and maintain commited field in the async commitMode
zhaoxugang Apr 24, 2021
0480254
Merge branch 'master' into first-transaction
zhaoxugang May 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions store/tikv/commit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,16 +133,26 @@ func (actionCommit) handleSingleBatch(c *twoPhaseCommitter, bo *Backoffer, batch
zap.Strings("keys", hexBatchKeys(keys)))
return errors.Trace(err)
}
// The transaction maybe rolled back by concurrent transactions.
logutil.Logger(bo.ctx).Debug("2PC failed commit primary key",
zap.Error(err),
zap.Uint64("txnStartTS", c.startTS))
if batch.isPrimary {
// The transaction maybe rolled back by concurrent transactions.
logutil.Logger(bo.ctx).Debug("2PC failed commit primary key",
zap.Error(err),
zap.Uint64("txnStartTS", c.startTS))
} else {
logutil.Logger(bo.ctx).Debug("2PC failed commit secondary key",
zap.Error(err),
zap.Uint64("txnStartTS", c.startTS))
}
return err
}

// if batch is not primary key then return immediately
// Group that contains primary key is always the first except async commit.
if !batch.isPrimary {
return nil
}
c.mu.Lock()
defer c.mu.Unlock()
// Group that contains primary key is always the first.
// We mark transaction's status committed when we receive the first success response.
c.mu.committed = true
Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to mark this field correctly when async commit protocol is used, as the cilent will receive OK packet when all prewrite requests are successfully processed. Then we may need to adjust the error log messages in L129, the debug log in L137 seems ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may need to mark this field correctly when async commit protocol is used, as the cilent will receive OK packet when all prewrite requests are successfully processed. Then we may need to adjust the error log messages in L129, the debug log in L137 seems ok.

I think the twoPhaseCommitter.mu.commited is true means the primary of batchs was commited,so L129 is ok,but in async commit ,the batch mabe not primary，so L137 mabe not accurate

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'd better maintain committed for async commit transactions (setting it in 2pc.go after prewriting finishes).
If we maintain `committed for async commit transactions, then "2PC failed commit key after primary key committed" would be inaccurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return nil
Expand Down