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: Remove gcTaskWorker #11033

Merged
merged 8 commits into from
Jul 4, 2019
Merged

store/tikv: Remove gcTaskWorker #11033

merged 8 commits into from
Jul 4, 2019

Conversation

MyonKeminta
Copy link
Contributor

Signed-off-by: MyonKeminta MyonKeminta@users.noreply.github.com

What problem does this PR solve?

The RangeTaskRunner is implemented just like how gcTaskWorker did, to suit for all tasks that need to do operations to regions in a range. Now let's remove gcTaskWorker and replace it with RangeTaskRunner.

What is changed and how it works?

This PR removed gcTaskWorker and uses RangeTaskRunner instead.

Check List

Tests

  • Unit test
  • Integration test

@MyonKeminta
Remove gcTaskWorker
052e82d 
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
disksing
disksing previously approved these changes Jul 2, 2019
View reviewed changes
Copy link
Contributor

@disksing disksing left a comment

Choose a reason for hiding this comment

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

Fantastic!

@codecov
Copy link

codecov bot commented Jul 2, 2019 •
edited
Loading

Codecov Report

Merging #11033 into master will increase coverage by 0.2118%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #11033        +/-   ##
================================================
+ Coverage   81.0036%   81.2155%   +0.2118%     
================================================
  Files           419        419                
  Lines         89375      90117       +742     
================================================
+ Hits          72397      73189       +792     
+ Misses        11736      11656        -80     
- Partials       5242       5272        +30

@MyonKeminta
Copy link
Contributor Author

/run-all-tests

@MyonKeminta
Copy link
Contributor Author

/run-unit-test

@MyonKeminta MyonKeminta dismissed disksing’s stale review July 2, 2019 09:39

Wait a minute. The metrics is not correct.

@MyonKeminta
Copy link
Contributor Author

The metrics is correct, but since RangeTaskRunner doesn't calculates number of failed regions, so the last log saying the number of failed regions doesn't work. I think I need to update RangeTaskRunner to support this.

jackysp
jackysp reviewed Jul 2, 2019
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

Please add some unit tests.

@MyonKeminta
Fix calculation of succeeded and failed regions which should be print…
795c73d 
…ed to log

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@MyonKeminta
Copy link
Contributor Author

@jackysp This is already tested by TestDoGC.

@jackysp
Copy link
Member

jackysp commented Jul 3, 2019

I found the coverage is a bit decreasing.

@MyonKeminta
Copy link
Contributor Author

@jackysp I think that's caused by removal of code which is previously covered. PTAL again.
Also, I'll do other works to improve the coverage, but it's not this PR.

@MyonKeminta
Copy link
Contributor Author

@disksing PTAL again. There's some more changes to let RangeTaskRunner able to calculate failed regions.

AndreMouche
AndreMouche approved these changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

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

LGTM. When would we support useDistributedGC only so we can remove the related code?

store/tikv/delete_range.go
startKey, rangeEndKey := r.StartKey, r.EndKey
completedRegions := 0
var stat RangeTaskStat
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think to define a variable without init it is a good idea, is it a good habit in golang?

Copy link
Contributor Author

@MyonKeminta MyonKeminta Jul 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's field will be initialized as 0 by default here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally good habit in Go.

@MyonKeminta
Copy link
Contributor Author

/run-all-tests

@MyonKeminta
Copy link
Contributor Author

/run-all-tests

@MyonKeminta MyonKeminta merged commit 0f1822a into pingcap:master Jul 4, 2019
@MyonKeminta MyonKeminta deleted the misono/remove-gctask-worker branch July 4, 2019 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackysp jackysp jackysp left review comments

@disksing disksing disksing approved these changes

@AndreMouche AndreMouche AndreMouche approved these changes

Assignees
No one assigned
Labels
component/tikv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MyonKeminta @jackysp @AndreMouche @disksing