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

*: wait for binlog recovering when using HTTP API #13740

Merged
merged 16 commits into from
Dec 3, 2019
Merged

*: wait for binlog recovering when using HTTP API #13740

merged 16 commits into from
Dec 3, 2019

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Nov 26, 2019 •
edited
Loading

What problem does this PR solve?

The current binlog/recover API does not wait for all transactions committed. We need to make the user knows when the binlog recovered for all running transactions.

What is changed and how it works?

  1. Wait for all transactions committed when using binlog/recover API and make it as the default behavior.
  2. Add the current implementation to an option of this API.
  3. Add a way to reset the current transaction counter.

Check List

Tests

  • Unit test
  • Manual test
  1. start a cluster with pump, set ignore-error = true
  2. run sysbench prepare data
  3. kill pump, the monitor will alert the binlog error
  4. use binlog/recover to recover the binlog, the alert will disappear.

Code changes

  • Has exported function/method change

Side effects

  • Possible performance regression

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Write release note for bug-fix or new feature.

@jackysp jackysp added the type/enhancement The issue or PR belongs to an enhancement. label Nov 26, 2019
@jackysp jackysp requested review from lysu and coocood November 26, 2019 08:03
@codecov
Copy link

codecov bot commented Nov 26, 2019 •
edited
Loading

Codecov Report

Merging #13740 into master will decrease coverage by 0.077%.
The diff coverage is 95.5555%.

@@               Coverage Diff               @@
##            master     #13740        +/-   ##
===============================================
- Coverage   80.135%   80.0579%   -0.0771%     
===============================================
  Files          475        473         -2     
  Lines       117624     116387      -1237     
===============================================
- Hits         94258      93177      -1081     
+ Misses       15906      15856        -50     
+ Partials      7460       7354       -106

coocood
coocood reviewed Nov 26, 2019
View reviewed changes
sessionctx/binloginfo/binloginfo.go Outdated
}

// AddOneCommitter adds one committer to committerCounter.
func AddOneCommitter() {
Copy link
Member

Choose a reason for hiding this comment

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

This is used for skipped binlog writer.
Better named to AddOneSkippedCommiter

@jackysp
check ignore error once
4044cc6 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp
address comments
a9d026a 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp
add test
8e50e02 
Signed-off-by: Shuaipeng Yu <[email protected]>
@coocood
Copy link
Member

coocood commented Nov 27, 2019

LGTM

@jackysp
tiny change
218a77e 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp
fix test
a5e3f83 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp
tiny change
8f9f321 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp
sleep first
24cebb0 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp
fix typo
0215986 
Signed-off-by: Shuaipeng Yu <[email protected]>
lysu
lysu reviewed Nov 28, 2019
View reviewed changes
store/tikv/2pc.go
} else {
c.writeFinishBinlog(ctx, binlog.BinlogType_Commit, int64(c.commitTS))
}
}
}()

binlogChan := c.prewriteBinlog(ctx)
prewriteBo := NewBackoffer(ctx, PrewriteMaxBackoff).WithVars(c.txn.vars)
Copy link
Contributor

Choose a reason for hiding this comment

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

if panic happened after prewriteBinlog before <-binlogChan will miss RemoveOneSkippedCommitter

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, can not avoid it, I've added a reset operation.

Copy link
Contributor

Choose a reason for hiding this comment

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

if there is in-flight txn, calling reset will make SkippedCommitterCounter < 0 finally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, reset is used for troubleshooting, if sth wrong with it, e.g. counter != 0 forever after resetting, we could reset it once again. I think we could not avoid it to happen, but hopefully, it will never be used.

@tiancaiamao
Copy link
Contributor

LGTM

@jackysp
address comments
bb209c4 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp
Copy link
Member Author

jackysp commented Nov 29, 2019

PTAL @lysu

@jackysp
unify return value
259d4fd 
Signed-off-by: Shuaipeng Yu <[email protected]>
@jackysp jackysp added status/LGT2 Indicates that a PR has LGTM 2. and removed status/DNM labels Nov 29, 2019
lysu
lysu previously approved these changes Dec 2, 2019
View reviewed changes
Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu
Copy link
Contributor

lysu commented Dec 2, 2019

@GregoryIan PTAL

@IANTHEREAL
Copy link
Contributor

@july2993 PTAL

@july2993 july2993 self-requested a review December 2, 2019 09:18
@jackysp
add status
33bb6a2 
Signed-off-by: Shuaipeng Yu <[email protected]>
july2993
july2993 reviewed Dec 3, 2019
View reviewed changes
Copy link
Contributor

@july2993 july2993 left a comment

Choose a reason for hiding this comment

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

LGTM

@lysu lysu added require-LGT3 Indicates that the PR requires three LGTM. status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. require-LGT3 Indicates that the PR requires three LGTM. labels Dec 3, 2019
@IANTHEREAL
Copy link
Contributor

LGTM

@tiancaiamao
Copy link
Contributor

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 3, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 3, 2019

/run-all-tests

@sre-bot sre-bot merged commit da414ef into pingcap:master Dec 3, 2019
@jackysp jackysp removed the status/DNM label Dec 4, 2019
@jackysp
Copy link
Member Author

jackysp commented Dec 4, 2019

/run-cherry-pick

@sre-bot
Copy link
Contributor

sre-bot commented Dec 4, 2019

cherry pick to release-3.0 failed

jackysp added a commit to jackysp/tidb that referenced this pull request Dec 4, 2019
@jackysp
*: wait for binlog recovering when using HTTP API (pingcap#13740)
2035911 
 Conflicts:
	store/tikv/2pc.go
@jackysp jackysp mentioned this pull request Dec 4, 2019
Merged
sre-bot pushed a commit that referenced this pull request Dec 6, 2019
@jackysp @sre-bot
*: wait for binlog recovering when using HTTP API (#13740) (#13892)
0201879
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@jackysp @XiaTianliang
*: wait for binlog recovering when using HTTP API (pingcap#13740)
f181b2b
@jackysp jackysp deleted the wait_binlog_recover branch February 27, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coocood coocood coocood left review comments

@july2993 july2993 july2993 left review comments

@lysu lysu lysu approved these changes

Assignees
No one assigned
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jackysp @coocood @tiancaiamao @lysu @IANTHEREAL @sre-bot @july2993