store/tikv: run gc workers parallelly #5837

zhangjinpeng87 · 2018-02-09T02:47:03Z

No description provided.

coocood · 2018-02-09T03:15:11Z

store/tikv/gcworker/gc_worker.go

+	return task, nil
+}
+
+func gcTaskWorker(store tikv.Storage, taskCh chan *gcTask, resultCh chan *gcResult) {


How about defining a struct type named gcTaskWorker, then add a method run?

Yes, I think it is a good idea.

wentaoxu · 2018-02-09T03:46:47Z

store/tikv/gcworker/gc_worker.go

-	bo := tikv.NewBackoffer(tikv.GcOneRegionMaxBackoff, ctx)
+	// create task queue and task result queue, and start task workers
+	gcTaskCh := make(chan *gcTask, gcConcurrency)
+	gcResultCh := make(chan *gcResult, 1024)


why 1024 here?

zhangjinpeng87 · 2018-02-09T06:16:19Z

store/tikv/gcworker/gc_worker.go

@@ -106,6 +106,10 @@ const (
 	gcDefaultLifeTime        = time.Minute * 10
 	gcSafePointKey           = "tikv_gc_safe_point"
 	gcSafePointCacheInterval = tikv.GcSafePointCacheInterval
+	gcConcurrencyKey         = "tikv_gc_concurrency"
+	gcDefaultConcurrency     = 32


As i have tested, the concurrency should less than 50% of the total count of TiKV nodes.

I test a cluster with 5 tikv node and 2000 regions, when i set GC concurrency as 2, only 5 regions was skipped during a round of GC.

It is somehow different from what I think it will do, but I'm glad it works well.

wentaoxu · 2018-02-09T08:34:27Z

PTAL @coocood @tiancaiamao @zhangjinpeng1987

tiancaiamao · 2018-02-09T07:53:25Z

store/tikv/gcworker/gc_worker.go

@@ -118,6 +123,7 @@ var gcVariableComments = map[string]string{
 	gcRunIntervalKey: "GC run interval, at least 10m, in Go format.",
 	gcLifeTimeKey:    "All versions within life time will not be collected by GC, at least 10m, in Go format.",
 	gcSafePointKey:   "All versions after safe point can be accessed. (DO NOT EDIT)",
+	gcConcurrencyKey: "How many go routines used to do GC parallel, [1, 1024], default 32",


default 32?

gcDefaultConcurrency = 2

tiancaiamao · 2018-02-09T08:02:59Z

store/tikv/gcworker/gc_worker.go

+}
+
+func (w *gcTaskWorker) run() {
+	for {


Don't use task == nil to indicate work channel finish.

Go idiom:

for task := <-w.taskCh { }

tiancaiamao · 2018-02-09T08:12:49Z

store/tikv/gcworker/gc_worker.go

+			res = &gcResult{
+				successRegions: 0,
+				failedRegions:  0,
+				lastErr:        err,


s/err/errors.Trace(err)
failedRegions = 0 when error is not nil?

It means we don't get the region, so set it 0 here.

tiancaiamao · 2018-02-09T08:15:07Z

store/tikv/gcworker/gc_worker.go

+				lastErr:        err,
+			}
+		}
+		if res != nil {


why not put it into line 565?

tiancaiamao · 2018-02-09T08:16:26Z

store/tikv/gcworker/gc_worker.go

+	}
+}
+
+func (w *GCWorker) loadGcConcurrencyWithDefault() (int, error) {


s/Gc/GC
Use capital letters for abbreviation

tiancaiamao · 2018-02-09T08:35:44Z

store/tikv/gcworker/gc_worker.go

 		case <-ticker.C:
 			log.Infof("[gc worker] %s gc in process, safePoint: %v, successful regions: %v, failed regions: %v, cost time: %s",
 				identifier, safePoint, successRegions, failedRegions, time.Since(startTime))
 		default:
 		}

+		bo := tikv.NewBackoffer(tikv.GcOneRegionMaxBackoff, ctx)
+		task, err := genNextGCTask(store, bo, safePoint, key)


key is not initialized?

start key is "", no need to do

tiancaiamao · 2018-02-09T08:36:21Z

store/tikv/gcworker/gc_worker.go

+		}
+	}
+
+	waitTaskWorkersDone(gcResultCh, concurrency)


tiancaiamao · 2018-02-09T08:40:59Z

store/tikv/gcworker/gc_worker.go

 		} else {
 			successRegions++
-			gcActionRegionResultCounter.WithLabelValues("success").Inc()
 		}

 		key = loc.EndKey


Is [startkey, endkey] a range and should we consider all regions in the range?

Yes, we do it in a range, not just a region if it splited

tiancaiamao · 2018-02-09T08:41:59Z

store/tikv/gcworker/gc_worker_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/pingcap/tidb/store/mockstore"
 	"github.com/pingcap/tidb/store/tikv"
 	goctx "golang.org/x/net/context"
+	"strconv"


Put standard packages and third-party packages in different code block.

tiancaiamao · 2018-02-09T08:43:51Z

store/tikv/gcworker/gc_worker.go

+		lastErr:        lastErr,
+	}
+
+	return res, nil
 }

 // these two errors should not return together, for more, see the func 'doGC'


This comment is strange, would you update it please?

It should be here, not outdated, we sadly rely on this.

coocood · 2018-02-09T08:51:34Z

store/tikv/gcworker/gc_worker.go

+			if taskRes == nil {
+				count++
+				if count == concurrency {
+					close(gcResultCh)


We don't need to deal with gcResultCh, its capacity is large enough, no task worker will be blocked on sending to this channel.

coocood · 2018-02-09T09:02:30Z

store/tikv/gcworker/gc_worker.go

+func (w *gcTaskWorker) run() {
+	defer w.wg.Done()
+	for {
+		task := <-w.taskCh


We can write the for loop like for task := range w.taskCh {
When the channel is closed, the for loop will break.

coocood · 2018-02-09T09:04:08Z

store/tikv/gcworker/gc_worker.go

+			return
+		}
+
+		res, err := doGCForRange(w.store, task.startKey, task.endKey, task.safePoint)


It would be cleaner to define the doGCForRange as a method of gcTaskWorker

wentaoxu · 2018-02-09T10:04:41Z

PTAL @zhangjinpeng1987 @coocood @tiancaiamao

zhangjinpeng87 · 2018-02-09T10:17:23Z

store/tikv/gcworker/gc_worker.go

+func (w *GCWorker) doGC(ctx goctx.Context, safePoint uint64) error {
+	gcConcurrency, err := w.loadGCConcurrencyWithDefault()
+	if err != nil {
+		log.Errorf("")


Miss err here.

zhangjinpeng87 · 2018-02-09T10:22:10Z

store/tikv/gcworker/gc_worker.go

+	ticker := time.NewTicker(gcJobLogTickInterval)
+	defer ticker.Stop()
+
+	// create task queue and task result queue, and start task workers


Please fix comment.

And sentence begins with a capital letter, end with dot.

zhangjinpeng87 · 2018-02-09T10:23:34Z

store/tikv/gcworker/gc_worker.go

+		gcHistogram.WithLabelValues("do_gc").Observe(time.Since(startTime).Seconds())
+	}()
+
+	bo := tikv.NewBackoffer(tikv.GcOneRegionMaxBackoff, ctx)


Please move this line into below for loop.

tiancaiamao · 2018-02-09T10:32:03Z

store/tikv/gcworker/gc_worker.go

@@ -37,6 +37,7 @@ import (
 	tidbutil "github.com/pingcap/tidb/util"
 	log "github.com/sirupsen/logrus"
 	goctx "golang.org/x/net/context"
+	"sync/atomic"


Move to line 23

tiancaiamao · 2018-02-09T10:44:09Z

store/tikv/gcworker/gc_worker.go

+	ticker := time.NewTicker(gcJobLogTickInterval)
+	defer ticker.Stop()
+
+	// create task queue and task result queue, and start task workers


And sentence begins with a capital letter, end with dot.

tiancaiamao · 2018-02-09T10:45:28Z

store/tikv/gcworker/gc_worker.go

+
+	// create task queue and task result queue, and start task workers
+	gcTaskCh := make(chan *gcTask, concurrency)
+	wg := sync.WaitGroup{}


var wg sync.WaitGroup wg.Add(concurrency)

tiancaiamao · 2018-02-09T10:55:29Z

store/tikv/gcworker/gc_worker.go

+		close(gcTaskCh)
+		wg.Wait()
+		log.Infof("[gc worker] %s finish gc, safePoint: %v, successful regions: %v, failed regions: %v, total cost time: %s",
+			identifier, safePoint, atomic.LoadInt32(&successRegions), atomic.LoadInt32(&failedRegions), time.Since(startTime))


It doesn't matter if it's not atomic load here, because when the code run here, no worker would use successRegions/failedRegions any more.
atomic load is okay, of course.

I'd like to keep it for future.

tiancaiamao · 2018-02-09T10:59:14Z

store/tikv/gcworker/gc_worker.go

+
+		task, err := genNextGCTask(store, bo, safePoint, key)
+		if err != nil {
+			log.Errorf("[gc worker] %s, gc interupted because get region error, err %v", identifier, errors.Trace(err))


s/errors.Trace(err) / errors.ErrorStack(err)

It will be reported in other, I think I'd better remove it.

tiancaiamao · 2018-02-09T11:00:08Z

store/tikv/gcworker/gc_worker.go

+		default:
+		}
+
+		task, err := genNextGCTask(store, bo, safePoint, key)


where is the variable key initialized?

what would happen when region split during different call of this function?

no need to init, default values "" is ok.

task is defined by range, not region, so the worker will deal with the two regions if it splited.

tiancaiamao · 2018-02-09T11:01:29Z

store/tikv/gcworker/gc_worker_test.go

@@ -123,33 +124,76 @@ func (s *testGCWorkerSuite) TestPrepareGC(c *C) {
 	safePoint, err = s.gcWorker.loadTime(gcSafePointKey, s.gcWorker.session)
 	c.Assert(err, IsNil)
 	s.timeEqual(c, safePoint.Add(time.Minute*30), now, 2*time.Second)
+
+	// change GC concurrency


tiancaiamao · 2018-02-09T11:15:26Z

store/tikv/gcworker/gc_worker.go

+func (w *gcTaskWorker) run() {
+	defer w.wg.Done()
+	for task := range w.taskCh {
+		err := w.doGCForRange(task.startKey, task.endKey, task.safePoint)


In region split case, will some worker GC the same region? Is there any problem for this situation?

No problem here, see above

tiancaiamao · 2018-02-09T11:16:12Z

store/tikv/gcworker/gc_worker.go

+	for task := range w.taskCh {
+		err := w.doGCForRange(task.startKey, task.endKey, task.safePoint)
+		if err != nil {
+			log.Errorf("[gc worker] %s, gc interupted because get region error, err %v", w.identifier, errors.Trace(err))


Better log it with the task info.

tiancaiamao · 2018-02-09T11:21:53Z

store/tikv/gcworker/gc_worker.go

+		gcConcurrency = gcDefaultConcurrency
+	}
+
+	return doGC(ctx, w.store, safePoint, w.uuid, gcConcurrency)


So many doGC makes me confused, could you change this function name to doGCParallelly

And this method can be merged to runGCJob

Empty function？

?this func is used to get the gcConcurrency

zhangjinpeng87 · 2018-02-09T12:17:15Z

LGTM

wentaoxu · 2018-02-09T12:17:32Z

/run-all-tests

zhangjinpeng87 · 2018-02-09T12:19:42Z

@coocood PTAL

tiancaiamao · 2018-02-09T12:31:42Z

LGTM @coocood

coocood · 2018-02-09T12:47:21Z

store/tikv/gcworker/gc_worker.go

+	return task, nil
+}
+
+func doGCParallel(ctx goctx.Context, store tikv.Storage, safePoint uint64, identifier string, concurrency int) error {


Make this function as a method of GCWorker is better.
We can remove RunGCJob as it is only used by a bench program.

coocood · 2018-02-09T13:10:56Z

LGTM

coocood · 2018-02-09T13:13:21Z

/run-all-tests

zhangjinpeng87

LGTM

run gc workers parallelly

9463c94

coocood reviewed Feb 9, 2018

View reviewed changes

fix bug

1945bd0

wentaoxu reviewed Feb 9, 2018

View reviewed changes

dynamically to change gcConcurrency

87693c9

zhangjinpeng87 commented Feb 9, 2018

View reviewed changes

add test and dynically change gcConcurrency

50cefba

wentaoxu changed the title ~~[DNM] run gc workers parallelly~~ store/tikv: run gc workers parallelly Feb 9, 2018

wentaoxu added 3 commits February 9, 2018 14:45

fix ut

4fe2f76

fix comments

8c51b33

Merge branch 'master' into zhangjinpeng/parallel-gc

9028003

fix channel length

0870b77

tiancaiamao reviewed Feb 9, 2018

View reviewed changes

coocood reviewed Feb 9, 2018

View reviewed changes

remove gcResultChan

3f58cb2

zhangjinpeng87 commented Feb 9, 2018

View reviewed changes

tiancaiamao reviewed Feb 9, 2018

View reviewed changes

wentaoxu added 3 commits February 9, 2018 19:59

fix comments

d078864

fix comments

b2dc868

Merge branch 'master' into zhangjinpeng/parallel-gc

59d44aa

fix comments

d46d475

coocood reviewed Feb 9, 2018

View reviewed changes

wentaoxu added 2 commits February 9, 2018 20:59

fix comments

d8aa1b5

fix comments

f4c459d

wentaoxu approved these changes Feb 9, 2018

View reviewed changes

wentaoxu merged commit a63601a into master Feb 9, 2018

zhangjinpeng87 commented Feb 9, 2018

View reviewed changes

ngaut deleted the zhangjinpeng/parallel-gc branch February 11, 2018 10:13

kennytm mentioned this pull request Oct 15, 2021

sessionctx/variable, executor: introduce a limit on "thread" config #28842

Merged

12 tasks

store/tikv: run gc workers parallelly #5837

store/tikv: run gc workers parallelly #5837

Conversation

zhangjinpeng87 commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangjinpeng87 Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wentaoxu commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wentaoxu commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangjinpeng87 commented Feb 9, 2018

wentaoxu commented Feb 9, 2018

zhangjinpeng87 commented Feb 9, 2018

tiancaiamao commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Feb 9, 2018

coocood commented Feb 9, 2018

zhangjinpeng87 left a comment

Choose a reason for hiding this comment

zhangjinpeng87 Feb 9, 2018 •

edited

Loading

tiancaiamao Feb 9, 2018 •

edited

Loading