store/tikv:Ignore error and do gc anyway #5797

wentaoxu · 2018-02-05T12:42:26Z

as title says

wentaoxu · 2018-02-05T12:42:38Z

PTAL @disksing @zhangjinpeng1987

disksing · 2018-02-05T12:47:26Z

store/tikv/gcworker/gc_worker.go

+			regions++
+		} else {
+			errRegions++
+		}


It is errRegions need ++ when err != nil.

disksing · 2018-02-05T12:49:48Z

store/tikv/gcworker/gc_worker.go

+			break
+		}
+	}
+	log.Infof("[gc worker] %s finish gc, safePoint: %v, regions: %v, ignored regions: %v, cost time: %s", identifier, safePoint, regions, errRegions, time.Since(startTime))


I think we can output this line of log at regular intervals (1min for example) during gc, so that we can know the GC progress.

zhangjinpeng87 · 2018-02-06T01:40:46Z

store/tikv/backoff.go

@@ -148,7 +148,7 @@ const (
 	getMaxBackoff           = 20000
 	prewriteMaxBackoff      = 20000
 	cleanupMaxBackoff       = 20000
-	GcMaxBackoff            = 100000
+	GcOneRegionMaxBackoff   = 20000


Why change this value?

I don't think gc command can definitely returns in 20 secs.

ok, I will change it for one request

zhangjinpeng87 · 2018-02-06T02:02:43Z

store/tikv/gcworker/gc_worker.go

+	return nil
+}
+
+func doGCForOneRegion(ctx goctx.Context, store tikv.Storage, safePoint uint64, identifier string, startKey []byte, endKey []byte) error {


use loc instead of startKey and endKey. By the way, you don't need endKey indeed.

Yes, I will use loc, for endKey, see below

func doGCForOneRegion(ctx goctx.Context, store tikv.Storage, safePoint uint64, identifier string, region RegionVerID) (RegionError, error)

zhangjinpeng87 · 2018-02-06T02:06:32Z

store/tikv/gcworker/gc_worker.go

 		key = loc.EndKey
-		if len(key) == 0 {
+		if len(key) == 0 || bytes.Compare(key, endKey) >= 0 {


I don't think L596-L599 should exist.

I want the func doGCForOneRegion used to cleanup one region, if we remove this condition, the func is confused.

zhangjinpeng87 · 2018-02-06T03:22:48Z

store/tikv/gcworker/gc_worker.go

-
-	var key []byte
+	bo := tikv.NewBackoffer(tikv.GcOneRegionMaxBackoff, goctx.Background())
+	key := startKey
 	for {


Please remove the loop.

zhangjinpeng87 · 2018-02-06T03:24:01Z

store/tikv/gcworker/gc_worker.go

+	return nil
+}
+
+func doGCForOneRegion(ctx goctx.Context, store tikv.Storage, safePoint uint64, identifier string, startKey []byte, endKey []byte) error {


func doGCForOneRegion(ctx goctx.Context, store tikv.Storage, safePoint uint64, identifier string, region RegionVerID) (RegionError, error)

wentaoxu · 2018-02-06T06:13:30Z

PTAL @disksing @zhangjinpeng1987 @tiancaiamao

zhangjinpeng87 · 2018-02-06T06:15:44Z

store/tikv/gcworker/gc_worker.go

-				return errors.Trace(err)
-			}
+			errRegions++
+		} else if regionErr != nil {


if regionErr != nil {
backoff
continue
}
if err != nil {
} else {
}

I think it is better to check err first, because if regionErr and err both exist, err should have higher priority.

zhangjinpeng87 · 2018-02-06T06:18:12Z

store/tikv/gcworker/gc_worker.go

+	ticker := time.NewTicker(gcJobLogTickInterval)
+	defer ticker.Stop()
+
+	bo := tikv.NewBackoffer(tikv.GcLocalRegionMaxBackoff, goctx.Background())


What does LocalRegionMaxBackoff mean?

I will remove it

zhangjinpeng87 · 2018-02-06T06:23:10Z

store/tikv/gcworker/gc_worker.go

 	gcHistogram.WithLabelValues("do_gc").Observe(time.Since(startTime).Seconds())
 	return nil
 }

+func doGCForOneRegion(store tikv.Storage, safePoint uint64, loc *tikv.KeyLocation) (*errorpb.Error, error) {
+	req := &tikvrpc.Request{


Seems you only need RegionVerID.

zhangjinpeng87 · 2018-02-06T07:04:12Z

store/tikv/gcworker/gc_worker.go

-		if gcResp.GetError() != nil {
-			return errors.Errorf("unexpected gc error: %s", gcResp.GetError())
+		} else {
+			regions++


if err != nil {
err++
}
if regionError != nil {
backoff
continue
}
if err == nil {
++}

wentaoxu · 2018-02-06T07:25:34Z

PTAL @zhangjinpeng1987

zhangjinpeng87 · 2018-02-06T07:50:15Z

store/tikv/gcworker/gc_worker.go

@@ -515,63 +516,95 @@ func doGC(ctx goctx.Context, store tikv.Storage, safePoint uint64, identifier st
 	// Sleep to wait for all other tidb instances update their safepoint cache.
 	time.Sleep(gcSafePointCacheInterval)

+	log.Infof("[gc worker] %s start gc, safePoint: %v.", identifier, safePoint)
+	startTime := time.Now()
+	regions := 0


s/regions/successRegions
s/errRegions/failedRegions

wentaoxu · 2018-02-06T08:13:16Z

/run-all-tests

wentaoxu · 2018-02-06T08:39:13Z

/run-all-tests

ngaut · 2018-02-06T08:40:39Z

Please add tests.

disksing · 2018-02-06T09:04:33Z

store/tikv/gcworker/gc_worker.go


+	bo := tikv.NewBackoffer(tikv.GcOneRegionMaxBackoff, ctx)


Initialize it in following for loop seems better.

It is hard to do because It should not be initialized when we meet region error.

zhangjinpeng87 · 2018-02-06T09:12:37Z

store/tikv/gcworker/gc_worker.go

 	gcHistogram.WithLabelValues("do_gc").Observe(time.Since(startTime).Seconds())
 	return nil
 }

+// these two errors should not return together, for more, see the func 'doGC'
+func doGCForOneRegion(bo *tikv.Backoffer, store tikv.Storage, req *tikvrpc.Request, region tikv.RegionVerID) (*errorpb.Error, error) {


Seems we should move the construction of req into this function?

I think for the completion, it should be move in here, but if we have 1M regions, this cost very large extra memory.

The cost is acceptable. If i pass an request not gc, the behavior is not consistent with the function name.

ok, I will change it

…idb into ignore_error_and_do_gc_anyway

wentaoxu · 2018-02-06T14:03:33Z

PTAL @zhangjinpeng1987

wentaoxu · 2018-02-06T14:20:55Z

@ngaut gc is hard to add test because of package dependency, I will refine the code and make it easier after this pr.

ngaut · 2018-02-06T14:25:17Z

Please use gofail to simulate error.

disksing · 2018-02-06T14:47:29Z

LGTM.

zhangjinpeng87 · 2018-02-06T15:09:43Z

LGTM

zhangjinpeng87 · 2018-02-07T01:56:28Z

store/tikv/gcworker/gc_worker.go

 	log.Infof("[gc worker] %s start gc, safePoint: %v.", identifier, safePoint)
 	startTime := time.Now()
-	regions := 0
+	successRegions := 0


It's better to add some metrics.

wentaoxu · 2018-02-07T02:20:33Z

@ngaut ok, I will do it

zhangjinpeng87 · 2018-02-07T07:45:07Z

store/tikv/gcworker/gc_worker.go

@@ -222,7 +224,7 @@ func (w *GCWorker) leaderTick(ctx goctx.Context) error {
 	}

 	w.gcIsRunning = true
-	log.Infof("[gc worker] %s starts GC job, safePoint: %v", w.uuid, safePoint)
+	log.Infof("[gc worker] %s starts the whole job, safePoint: %v", w.uuid, safePoint)


I change the log because it is too similar to the log in the func doGC.

wentaoxu · 2018-02-07T08:00:58Z

PTAL @ngaut

ngaut

LGTM

* ignore error when gc, and continue do gc next region

wentaoxu added 2 commits February 5, 2018 20:38

ignore error when gc, and do gc on next region

d1c6181

add ignored regions

774980e

wentaoxu added the DNM label Feb 5, 2018

disksing reviewed Feb 5, 2018

View reviewed changes

zhangjinpeng87 reviewed Feb 6, 2018

View reviewed changes

add log ticker

3a09833

zhangjinpeng87 reviewed Feb 6, 2018

View reviewed changes

wentaoxu added 2 commits February 6, 2018 13:46

fix comments

1f724ac

fix ctx

68c8f4b

zhangjinpeng87 reviewed Feb 6, 2018

View reviewed changes

fix comments, add log and metrics

65ef1bf

zhangjinpeng87 reviewed Feb 6, 2018

View reviewed changes

fix comments

971928b

wentaoxu changed the title ~~[DNM]Ignore error and do gc anyway~~ Ignore error and do gc anyway Feb 6, 2018

wentaoxu removed the DNM label Feb 6, 2018

zhangjinpeng87 reviewed Feb 6, 2018

View reviewed changes

fix comments

f54618f

Merge branch 'master' into ignore_error_and_do_gc_anyway

68b5a60

disksing reviewed Feb 6, 2018

View reviewed changes

zhangjinpeng87 reviewed Feb 6, 2018

View reviewed changes

wentaoxu added 2 commits February 6, 2018 21:21

fix comments

6702d83

Merge branch 'ignore_error_and_do_gc_anyway' of github.com:wentaoxu/t…

4112b67

…idb into ignore_error_and_do_gc_anyway

Merge branch 'master' into ignore_error_and_do_gc_anyway

30516f1

wentaoxu changed the title ~~Ignore error and do gc anyway~~ store/tikv:Ignore error and do gc anyway Feb 6, 2018

disksing added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 6, 2018

zhangjinpeng87 added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 6, 2018

zhangjinpeng87 reviewed Feb 7, 2018

View reviewed changes

ngaut removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 7, 2018

wentaoxu added 3 commits February 7, 2018 14:41

add test

9e0c930

add test and active gc worker test

e02f351

fix fmt

8360650

zhangjinpeng87 reviewed Feb 7, 2018

View reviewed changes

Merge branch 'master' into ignore_error_and_do_gc_anyway

afd9c75

wentaoxu added 2 commits February 7, 2018 16:34

fix comments

d24eb68

fix comments

3d0155a

ngaut approved these changes Feb 7, 2018

View reviewed changes

ngaut merged commit c6046b2 into pingcap:master Feb 7, 2018

wentaoxu added a commit to wentaoxu/tidb that referenced this pull request Feb 7, 2018

store/tikv:Ignore error and do gc anyway (pingcap#5797)

c642f87

* ignore error when gc, and continue do gc next region

coocood pushed a commit that referenced this pull request Feb 7, 2018

store/tikv:Ignore error and do gc anyway (#5797) (#5815)

cbf90f4

* ignore error when gc, and continue do gc next region

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

store/tikv:Ignore error and do gc anyway #5797

store/tikv:Ignore error and do gc anyway #5797

Conversation

wentaoxu commented Feb 5, 2018

wentaoxu commented Feb 5, 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

zhangjinpeng87 Feb 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wentaoxu commented Feb 6, 2018

zhangjinpeng87 Feb 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wentaoxu commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wentaoxu commented Feb 6, 2018

wentaoxu commented Feb 6, 2018

ngaut commented Feb 6, 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

wentaoxu commented Feb 6, 2018

wentaoxu commented Feb 6, 2018 • edited Loading

ngaut commented Feb 6, 2018

disksing commented Feb 6, 2018

zhangjinpeng87 commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wentaoxu commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wentaoxu commented Feb 7, 2018

ngaut left a comment

Choose a reason for hiding this comment

zhangjinpeng87 Feb 6, 2018 •

edited

Loading

zhangjinpeng87 Feb 6, 2018 •

edited

Loading

wentaoxu commented Feb 6, 2018 •

edited

Loading