Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

restore: update and restore GCLifeTime once when parallel #220

Merged
merged 14 commits into from
Aug 7, 2019

Conversation

lance6716
Copy link
Contributor

@lance6716 lance6716 commented Jul 29, 2019

What problem does this PR solve?

DoChecksum doesn't prepare for parallel case

What is changed and how it works?

There may be multiple DoChecksum running, setting and resetting GC Life Time should not be done when there's unfinished DoChecksum tasks.

Lightning have restoreTables runs simultaneously, so we use this logic

func restoreTables() {
    // init_helper
    for some conurrency {
        go restoreTable()
    }
}

func restoreTable() {
    // call postProcess() -> ... -> DoChecksum()
}

func DoChecksum() {
    // using helper's pointers of lock, running jobs counter, original value
}

Using variable to count running checksum jobs. Before a remote chechsum call starting, lock, increase this counter, check if it just rised from zero then backup original value and call set GC Life Time logic, unlock.
After a remote chechsum finishing, lock, decrease this counter, check if it just drop to zero then call resetting GC Life Time logic, unlock.

The lock, counter, original value are pointed to same per location in one restoreTables.

Check List

Tests

  • Unit test
  • Integration test

Side effects

Related changes

  • Need to cherry-pick to the release branch

@lance6716 lance6716 added 2.1-cherry-pick The target branch is release-2.1 status/PTAL This PR is ready for review. Add this label back after committing new changes priority/normal type/bug-fix Bug fix type/bug This issue is a bug report 3.0-cherry-pick The target branch is release-3.0 Should Cherry-pick This PR should be cherry-picked back the previous release train and removed 2.1-cherry-pick The target branch is release-2.1 3.0-cherry-pick The target branch is release-3.0 labels Jul 29, 2019
@lance6716
Copy link
Contributor Author

Seems that it's another solution of #218. Should I borrow the code and integration test and revert context-related code? @kennytm @amyangfei

@kennytm
Copy link
Collaborator

kennytm commented Jul 29, 2019

🤔 Oh I thought you've found another test case which isn't fixed by #218.

An advantage of the Context based solution is that it doesn't rely on global variables.

@lance6716
Copy link
Contributor Author

🤔 Oh I thought you've found another test case which isn't fixed by #218.

An advantage of the Context based solution is that it doesn't rely on global variables.

I'll learn Context and fix that conflict tomorrow.

@lance6716 lance6716 added status/WIP Work in progress and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jul 30, 2019
@lance6716
Copy link
Contributor Author

@leoppro found a bug in my atomic integer code.

C1:->atomic check---------------------------------------->set GC->
C2:--------------->atomic check--->remote checksum call---------->

We think there should be a mutex, so I'll add three variable: a mutex for set GC time, a atomic integer as job counter(or a mutex for reset GC time), original GC time.

I still prefer using global variable, since lightning can only deal with one TiDB, and three variables are about "lock for TiDB setting, TiDB running jobs, TiDB setting" accrodingly, global variable won't pollute a lot.

Another way is using context like #218, assign three-variable-struct to context in restoreTables and get values in DoChecksum.

I'd like to learn your suggestions.

@kennytm
Copy link
Collaborator

kennytm commented Jul 30, 2019

@lance6716 use a struct, related variables should be placed close to each other.

Merge branch 'master' into fix-gc-life-time
Discussion see #220
@lance6716
Copy link
Contributor Author

/run-all-tests

@lance6716
Copy link
Contributor Author

PTAL @kennytm @amyangfei

@lance6716 lance6716 added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP Work in progress labels Jul 30, 2019
@lance6716
Copy link
Contributor Author

PTAL @leoppro

lightning/restore/restore.go Outdated Show resolved Hide resolved
lightning/restore/restore.go Outdated Show resolved Hide resolved
lightning/restore/restore.go Outdated Show resolved Hide resolved
lightning/restore/restore.go Outdated Show resolved Hide resolved
@lance6716
Copy link
Contributor Author

/run-all-tests

lightning/restore/restore.go Outdated Show resolved Hide resolved
lightning/restore/restore.go Outdated Show resolved Hide resolved
@lance6716
Copy link
Contributor Author

/run-all-tests

Copy link
Collaborator

@kennytm kennytm 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

lightning/restore/restore.go Show resolved Hide resolved
lightning/restore/restore.go Show resolved Hide resolved
@lance6716
Copy link
Contributor Author

/run-all-tests

lightning/restore/restore.go Outdated Show resolved Hide resolved
lightning/restore/restore_test.go Outdated Show resolved Hide resolved
@lance6716
Copy link
Contributor Author

/run-all-tests

Copy link
Collaborator

@kennytm kennytm 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

lightning/restore/restore_test.go Outdated Show resolved Hide resolved
@lance6716
Copy link
Contributor Author

/run-all-tests

@leoppro PTAL

@lance6716 lance6716 added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Aug 5, 2019
@kennytm
Copy link
Collaborator

kennytm commented Aug 6, 2019

Ping @leoppro

lightning/restore/restore.go Show resolved Hide resolved
lightning/restore/restore.go Show resolved Hide resolved
Copy link
Contributor

@zier-one zier-one left a comment

Choose a reason for hiding this comment

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

LGTM

@lance6716 lance6716 merged commit e8d463a into master Aug 7, 2019
@lance6716 lance6716 deleted the fix-gc-life-time branch August 7, 2019 01:46
@lance6716 lance6716 added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Aug 7, 2019
lance6716 added a commit that referenced this pull request Aug 7, 2019
* restore: update and restore GCLifeTime once when parallel

* Update lightning/restore/restore.go

Co-Authored-By: amyangfei <[email protected]>

* Update lightning/restore/restore.go

Co-Authored-By: amyangfei <[email protected]>

* Update lightning/restore/restore.go

Co-Authored-By: amyangfei <[email protected]>

* restore: fix bug in reviewing

* restore: call ObtainGCLifeTime closer to use

* restore: improve readabiilty

* restore: reduce struct copy and pointer deref

* restore: fix bug in reviewing

* Update lightning/restore/restore.go

Co-Authored-By: kennytm <[email protected]>

* restore: improve readability

* restore: refine mock expectation order

* restore: adjust detail
lance6716 added a commit that referenced this pull request Aug 7, 2019
* restore: update and restore GCLifeTime once when parallel

* Update lightning/restore/restore.go

Co-Authored-By: amyangfei <[email protected]>

* Update lightning/restore/restore.go

Co-Authored-By: amyangfei <[email protected]>

* Update lightning/restore/restore.go

Co-Authored-By: amyangfei <[email protected]>

* restore: fix bug in reviewing

* restore: call ObtainGCLifeTime closer to use

* restore: improve readabiilty

* restore: reduce struct copy and pointer deref

* restore: fix bug in reviewing

* Update lightning/restore/restore.go

Co-Authored-By: kennytm <[email protected]>

* restore: improve readability

* restore: refine mock expectation order

* restore: adjust detail
lance6716 added a commit that referenced this pull request Aug 7, 2019
lance6716 added a commit that referenced this pull request Aug 7, 2019
* restore: update and restore GCLifeTime once when parallel

* Update lightning/restore/restore.go

Co-Authored-By: amyangfei <[email protected]>

* Update lightning/restore/restore.go

Co-Authored-By: amyangfei <[email protected]>

* Update lightning/restore/restore.go

Co-Authored-By: amyangfei <[email protected]>

* restore: fix bug in reviewing

* restore: call ObtainGCLifeTime closer to use

* restore: improve readabiilty

* restore: reduce struct copy and pointer deref

* restore: fix bug in reviewing

* Update lightning/restore/restore.go

Co-Authored-By: kennytm <[email protected]>

* restore: improve readability

* restore: refine mock expectation order

* restore: adjust detail
kennytm pushed a commit that referenced this pull request Aug 7, 2019
* restore: update and restore GCLifeTime once when parallel (#220)
@kennytm kennytm removed the Should Cherry-pick This PR should be cherry-picked back the previous release train label Aug 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) type/bug This issue is a bug report type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants