store/tikv: Avoid sending to channel everywhere in runGCJob#11032
Conversation
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
|
@disksing @zhangjinpeng1987 PTAL. |
| if err != nil { | ||
| metrics.GCJobFailureCounter.WithLabelValues("prepare").Inc() | ||
| } | ||
| w.gcIsRunning = false |
There was a problem hiding this comment.
I think removing it will cause gcIsRunning cannot flip to false any more.
There was a problem hiding this comment.
If i'm right, here w.gcIsRunning must be false because of the check at L227. The only situation of changing it to false is at L179, and w.done is sent at L262. Please double check.
There was a problem hiding this comment.
oh, you are right. I made a mistake.
There was a problem hiding this comment.
Does other routine may change the w.gcIsRunning?
|
/run-all-tests |
Codecov Report
@@ Coverage Diff @@
## master #11032 +/- ##
===========================================
Coverage 81.7314% 81.7314%
===========================================
Files 423 423
Lines 92038 92038
===========================================
Hits 75224 75224
Misses 11514 11514
Partials 5300 5300 |
|
/run-unit-test |
|
@disksing Let's merge this PR later. I found that the existing tests are not sufficient to ensure this PR's correctness and I'll add some more tests in another PR. |
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
MyonKeminta
dismissed stale reviews from zhangjinpeng87 and disksing
via
6842992
|
/run-all-tests |
|
/run-all-tests |
|
/run-all-tests |
MyonKeminta
added
component/GC
status/LGT1
|
/run-unit-test |
MyonKeminta
changed the title
3 participants
Signed-off-by: MyonKeminta MyonKeminta@users.noreply.github.com
What problem does this PR solve?
The code in GCWorker doesn't looks good. I'm trying to refine it.
What is changed and how it works?
This refinement includes two points:
w.gcIsRunningseems should be set tofalsewhen the main loop receives fromw.done. So other assignment ofw.gcIsRunningis not useful.runGCJobis not in the main loop but a separated goroutine. When it finishes, it should send a signal tow.done. However there are many returns inrunGCJob. There's a risk that someone changes the code, added a return but forgot to send the signal. It seems to be ok to put it in adeferclause, however there's another risk that a:=is used in a inner scope, causing the error sent tow.doneis not the actual error. So I returned the error from the function and send the error outside the function.Check List
Tests