Skip to content
This repository has been archived by the owner on Aug 21, 2023. It is now read-only.

fix consistency bug #128

Merged
merged 4 commits into from
Aug 7, 2020
Merged

fix consistency bug #128

merged 4 commits into from
Aug 7, 2020

Conversation

lichunzhu
Copy link
Contributor

@lichunzhu lichunzhu commented Aug 6, 2020

What problem does this PR solve?

  1. When dumpling use to unlock tables; to unlock the FTWRL lock it actually doesn't take effect. Because dumpling use sql.DB to unlock tables but FTWRL need to be unlocked by the original connection.
  2. Currently metadata is recorded after FTWRL is released which is not proper. Metadata info will not be affected by consistent snapshot, so we should record it before FTWRL is released.

What is changed and how it works?

  1. Use one single connection to use FTWRL.
  2. Add consistency integration test.

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Fix the bug that FTWRL lock is not released in time.

@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #128 into master will increase coverage by 0.02%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
+ Coverage   50.75%   50.78%   +0.02%     
==========================================
  Files          17       17              
  Lines        1909     1910       +1     
==========================================
+ Hits          969      970       +1     
- Misses        868      869       +1     
+ Partials       72       71       -1     

v4/export/sql.go Outdated
func FlushTableWithReadLock(db *sql.DB) error {
_, err := db.Exec("FLUSH TABLES WITH READ LOCK")
func FlushTableWithReadLock(db *sql.Conn) error {
_, err := db.ExecContext(context.Background(), "FLUSH TABLES WITH READ LOCK")
Copy link
Member

Choose a reason for hiding this comment

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

Please avoiding context.Background().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed, PTAL again

serverType ServerType
db *sql.DB
conn *sql.Conn
}

func (c *ConsistencyFlushTableWithReadLock) Setup() error {
Copy link
Member

Choose a reason for hiding this comment

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

What about

Suggested change
func (c *ConsistencyFlushTableWithReadLock) Setup() error {
func (c *ConsistencyFlushTableWithReadLock) Setup(ctx context.Context) error {

}

type ConsistencyLockDumpingTables struct {
db *sql.DB
ctx context.Context
Copy link
Member

Choose a reason for hiding this comment

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


# dumping with consistency flush
export DUMPLING_TEST_DATABASE=$DB_NAME
export GO_FAILPOINTS="github.com/pingcap/dumpling/v4/export/ConsistencyCheck=return(\"5s\")"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export GO_FAILPOINTS="github.com/pingcap/dumpling/v4/export/ConsistencyCheck=return(\"5s\")"
export GO_FAILPOINTS="github.com/pingcap/dumpling/v4/export/ConsistencyCheck=1*sleep(5000)"

Comment on lines 169 to 177
failpoint.Inject("ConsistencyCheck", func(val failpoint.Value) {
interval, err := time.ParseDuration(val.(string))
if err != nil {
log.Warn("inject failpoint ConsistencyCheck failed", zap.Reflect("value", val), zap.Error(err))
} else {
log.Info("start to sleep for failpoint ConsistencyCheck", zap.Duration("sleepTime", interval))
time.Sleep(interval)
}
})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
failpoint.Inject("ConsistencyCheck", func(val failpoint.Value) {
interval, err := time.ParseDuration(val.(string))
if err != nil {
log.Warn("inject failpoint ConsistencyCheck failed", zap.Reflect("value", val), zap.Error(err))
} else {
log.Info("start to sleep for failpoint ConsistencyCheck", zap.Duration("sleepTime", interval))
time.Sleep(interval)
}
})
failpoint.Inject("consistency-check", nil)

Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm merged commit 6f74c68 into pingcap:master Aug 7, 2020
@kennytm kennytm added the status/LGT1 One reviewer approved (LGTM1) label Aug 7, 2020
@lichunzhu lichunzhu deleted the fixConsistencyBug branch August 7, 2020 07:06
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
* fix consistency problem

* add consistency integration test

* address comment

* address comments
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
* fix consistency problem

* add consistency integration test

* address comment

* address comments
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
* fix consistency problem

* add consistency integration test

* address comment

* address comments
tisonkun pushed a commit to tisonkun/dumpling that referenced this pull request Oct 20, 2021
* fix consistency problem

* add consistency integration test

* address comment

* address comments
tisonkun pushed a commit to tisonkun/tidb that referenced this pull request Oct 20, 2021
* fix consistency problem

* add consistency integration test

* address comment

* address comments
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT1 One reviewer approved (LGTM1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants