dm-worker: refine db operation #236

3pointer · 2019-08-12T12:46:43Z

What problem does this PR solve?

Unify db operation in Syncer and Loader into one pkg
Remove syncer.maxRetry config

What is changed and how it works?

Use baseConn struct to connect db, both Syncer and Loader hold the baseConn, which can take same retry policy in the future.
Remove syncer.maxRetry config, In my option, if we can define error type in the future, when we met error we need to retry, we should retry. user may not realize the error we met, and don't know how many retry times need to be set, So make the user controlling the maxRetry times is a bit improperly.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

3pointer · 2019-08-13T02:40:43Z

/run-all-tests

3pointer · 2019-08-13T03:07:56Z

/run-all-tests

3pointer · 2019-08-13T06:51:46Z

/run-all-tests

3pointer · 2019-08-13T11:05:44Z

/run-all-tests

3pointer · 2019-08-14T02:25:44Z

/run-all-tests

3pointer · 2019-08-14T03:26:42Z

/run-all-tests

3pointer · 2019-08-14T08:37:47Z

/run-all-tests

codecov · 2019-08-14T08:44:04Z

Codecov Report

Merging #236 into master will increase coverage by 1.195%.
The diff coverage is 62.6582%.

@@               Coverage Diff               @@
##             master       #236       +/-   ##
===============================================
+ Coverage   58.6197%   59.8148%   +1.195%     
===============================================
  Files           125        128        +3     
  Lines         15360      14366      -994     
===============================================
- Hits           9004       8593      -411     
+ Misses         5468       4924      -544     
+ Partials        888        849       -39

3pointer · 2019-08-15T08:17:53Z

/run-all-tests

IANTHEREAL · 2019-08-15T12:52:15Z

pkg/utils/db.go

+func (conn *BaseConn) NormalRetryOperation(ctx *tcontext.Context, operateFn func(*tcontext.Context, int) (interface{}, error), retryFn func(int, error) bool) (interface{}, error) {
+	var err error
+	var ret interface{}
+	for i := 0; i < 100; i++ {


please don't use hard code

just let retryFn to control when to exit?

retryFn may also need a tcontext argument

should we sleep a duration?

IANTHEREAL · 2019-08-15T13:00:35Z

pkg/utils/db.go

+}
+
+// InfinityRetryOperation retry operation until succeed
+func (conn *BaseConn) InfinityRetryOperation(operateFn func() (interface{}, error), retryFn func(error) bool) (interface{}, error) {


do operateFn and retryFn need tcontext argument?

IANTHEREAL · 2019-08-15T14:33:10Z

pkg/utils/db.go

+
+// QuerySQL query sql
+func (conn *BaseConn) QuerySQL(tctx *tcontext.Context, query string) (*sql.Rows, error) {
+	if conn == nil || conn.DB == nil {


should we also check it in NormalRetryOperation and InfinityRetryOperation？

IANTHEREAL · 2019-08-15T14:46:54Z

pkg/utils/db.go

+				tctx.L().Error("rollback failed", zap.String("Query", sqls[i].Query), zap.Reflect("argument", sqls[i].Args), log.ShortError(rerr))
+			}
+			// we should return the exec err, instead of the rollback rerr.
+			return i, errors.Trace(err)


should we use the new error system in #216？

this PR need rebase master after #216 merged

IANTHEREAL · 2019-08-15T14:55:23Z

syncer/db.go

 	"github.com/pingcap/dm/dm/config"
 	tcontext "github.com/pingcap/dm/pkg/context"
 	"github.com/pingcap/dm/pkg/log"
+	"github.com/pingcap/dm/pkg/utils"


We divide the import package path into three sections

built-in packages

dm packages

other packages

IANTHEREAL · 2019-08-15T15:08:42Z

syncer/relay.go

@@ -60,7 +60,7 @@ func (s *Syncer) setInitActiveRelayLog() error {
 		}
 	} else {
 		// start from dumper or loader, get current pos from master
-		pos, _, err = utils.GetMasterStatus(s.fromDB.db, s.cfg.Flavor)
+		pos, _, err = utils.GetMasterStatus(s.fromDB.baseConn.DB, s.cfg.Flavor)


wrap a function to make it like s.fromDB.GetMasterStatus?

IANTHEREAL · 2019-08-15T15:21:25Z

loader/db.go

-func (conn *Conn) executeSQL2(ctx *tcontext.Context, stmt string, maxRetry int, args ...interface{}) error {
-	if conn == nil || conn.db == nil {
-		return errors.NotValidf("database connection")
+func (conn *Conn) executeSQL(ctx *tcontext.Context, ddl bool, queries []string, args ...[]interface{}) error {


can we replace ddl bool argument by other argument like retryFn?

IANTHEREAL · 2019-08-15T15:36:46Z

how to reset all connections in one unit to handle error invalid connection/bad connection?

WangXiangUSTC · 2019-08-16T07:21:54Z

pkg/utils/db.go

+}
+
+// InfinityRetryOperation retry operation until succeed
+func (conn *BaseConn) InfinityRetryOperation(operateFn func() (interface{}, error), retryFn func(error) bool) (interface{}, error) {


seems this function has no relationship with BaseConn?

yes, its useless, removed already

pkg/utils/db.go

syncer/db.go

IANTHEREAL · 2019-08-16T14:24:34Z

one important question: under what circumstances we will receive an invalid connection error？you can learn from code of go-mysql.
And I think we need to have a strategy to avoid the situation that cannot do replication because of frequent invalid connection. We prefer slow replication than going into the error recovery process frequently (because binlog replication unit doesn't save checkpoint ASAP, it would re-replicate from old checkpoint again and again)

pkg/baseconn/conn_test.go

csuzhangxc

rest LGTM.
also see #236 (comment) again.

pkg/baseconn/conn.go

pkg/retry/strategy.go

pkg/retry/strategy_test.go

loader/db.go

syncer/db.go

syncer/syncer_test.go

tests/load_interrupt/run.sh

csuzhangxc · 2019-08-25T02:22:39Z

how about adding slow log (if cost > 1) for syncer/db.go?

3pointer · 2019-08-25T02:27:28Z

how about adding slow log (if cost > 1) for syncer/db.go?

you mean log as same as loader/db.go such like ctx.L().Warn("transaction execute successfully", zap.Duration("cost time", cost))?

csuzhangxc · 2019-08-25T02:28:49Z

how about adding slow log (if cost > 1) for syncer/db.go?

you mean log as same as loader/d.go such like ctx.L().Warn("transaction execute successfully", zap.Duration("cost time", cost))?

YES, it is.

3pointer · 2019-08-25T02:34:36Z

how about adding slow log (if cost > 1) for syncer/db.go?

you mean log as same as loader/d.go such like ctx.L().Warn("transaction execute successfully", zap.Duration("cost time", cost))?

YES, it is.

ok, fixed

csuzhangxc

LGTM

syncer/db.go

amyangfei

LGTM

* syncer db conn * loader db refine * remove useless code * add db retry policy * remove syncer max retry config * add normal retry policy * update retry strategy * add context for retry * update to terror * add invalid strategy * update retry invalid connection * update retry strategy * add retry strategy comment * add reset connection logic * use same baseConn in Conns * udpate syncer dbs * unify query in log * share same *DB by all conns * split load execute sql with different retryFn * unify retry logic to pkg * put retry error together * unify syncer conn helper function * update retry logic and some tests error formats * add baseConn test * make baseconn a pkg * abstract retry strategy * add querySQL args * add whiteList error retry logic * unify retry error in syncer/loader * keep max-retry in syncerConifg

3pointer added status/WIP This PR is still work in progress type/feature New feature type/feature-request This issue is a feature request labels Aug 12, 2019

3pointer force-pushed the refine_db_operation branch 2 times, most recently from 5b2d3e2 to a0ad3d3 Compare August 13, 2019 07:59

3pointer force-pushed the refine_db_operation branch from a2cb07a to 837e0a4 Compare August 14, 2019 07:56

3pointer added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Aug 15, 2019

3pointer changed the title ~~Refine db operation~~ dm-worker: refine db operation Aug 15, 2019

3pointer added the priority/important Major change, requires approval from ≥2 primary reviewers label Aug 15, 2019

IANTHEREAL reviewed Aug 15, 2019

View reviewed changes

WangXiangUSTC reviewed Aug 16, 2019

View reviewed changes

tmp try

ae89748

keep max-retry in syncerConifg

b08802f

amyangfei reviewed Aug 24, 2019

View reviewed changes

pkg/baseconn/conn_test.go Show resolved Hide resolved

3pointer added 3 commits August 24, 2019 09:44

address comment

8d045c2

address comments

0167f13

fix ci

897d469

IANTHEREAL removed the status/PTAL This PR is ready for review. Add this label back after committing new changes label Aug 24, 2019

csuzhangxc reviewed Aug 24, 2019

View reviewed changes

3pointer added 5 commits August 24, 2019 21:46

address comments

4802b2a

udpate comment

7749b34

udpate comments

a17395b

address comment

7a258ec

address comments

9133854

address comments

eb580c7

address comment

d78e0d5

csuzhangxc reviewed Aug 25, 2019

View reviewed changes

csuzhangxc added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Aug 25, 2019

amyangfei reviewed Aug 25, 2019

View reviewed changes

syncer/db.go Outdated Show resolved Hide resolved

amyangfei reviewed Aug 25, 2019

View reviewed changes

syncer/db.go Outdated Show resolved Hide resolved

address comment

e9efcb2

amyangfei approved these changes Aug 25, 2019

View reviewed changes

amyangfei added status/LGT3 Three reviewers already commented LGTM, ready for merge and removed status/LGT2 Two reviewers already commented LGTM, ready for merge labels Aug 25, 2019

3pointer merged commit 2ed7cb6 into pingcap:master Aug 26, 2019

3pointer deleted the refine_db_operation branch August 26, 2019 02:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dm-worker: refine db operation #236

dm-worker: refine db operation #236

3pointer commented Aug 12, 2019 •

edited

Loading

3pointer commented Aug 13, 2019

3pointer commented Aug 13, 2019

3pointer commented Aug 13, 2019

3pointer commented Aug 13, 2019

3pointer commented Aug 14, 2019

3pointer commented Aug 14, 2019

3pointer commented Aug 14, 2019

codecov bot commented Aug 14, 2019 •

edited

Loading

3pointer commented Aug 15, 2019

IANTHEREAL Aug 15, 2019

IANTHEREAL Aug 15, 2019

IANTHEREAL Aug 15, 2019 •

edited

Loading

IANTHEREAL Aug 15, 2019 •

edited

Loading

IANTHEREAL Aug 15, 2019

IANTHEREAL Aug 15, 2019

IANTHEREAL Aug 15, 2019 •

edited

Loading

3pointer Aug 19, 2019

IANTHEREAL Aug 15, 2019 •

edited

Loading

3pointer Aug 19, 2019

IANTHEREAL Aug 15, 2019

IANTHEREAL Aug 15, 2019 •

edited

Loading

IANTHEREAL commented Aug 15, 2019

WangXiangUSTC Aug 16, 2019

3pointer Aug 19, 2019

IANTHEREAL commented Aug 16, 2019 •

edited

Loading

csuzhangxc left a comment

csuzhangxc commented Aug 25, 2019

3pointer commented Aug 25, 2019 •

edited

Loading

csuzhangxc commented Aug 25, 2019

3pointer commented Aug 25, 2019

csuzhangxc left a comment

amyangfei left a comment

dm-worker: refine db operation #236

dm-worker: refine db operation #236

Conversation

3pointer commented Aug 12, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

3pointer commented Aug 13, 2019

3pointer commented Aug 13, 2019

3pointer commented Aug 13, 2019

3pointer commented Aug 13, 2019

3pointer commented Aug 14, 2019

3pointer commented Aug 14, 2019

3pointer commented Aug 14, 2019

codecov bot commented Aug 14, 2019 • edited Loading

Codecov Report

3pointer commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IANTHEREAL Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

IANTHEREAL Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IANTHEREAL Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IANTHEREAL Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IANTHEREAL Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

IANTHEREAL commented Aug 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IANTHEREAL commented Aug 16, 2019 • edited Loading

csuzhangxc left a comment

Choose a reason for hiding this comment

csuzhangxc commented Aug 25, 2019

3pointer commented Aug 25, 2019 • edited Loading

csuzhangxc commented Aug 25, 2019

3pointer commented Aug 25, 2019

csuzhangxc left a comment

Choose a reason for hiding this comment

amyangfei left a comment

Choose a reason for hiding this comment

3pointer commented Aug 12, 2019 •

edited

Loading

codecov bot commented Aug 14, 2019 •

edited

Loading

IANTHEREAL Aug 15, 2019 •

edited

Loading

IANTHEREAL Aug 15, 2019 •

edited

Loading

IANTHEREAL Aug 15, 2019 •

edited

Loading

IANTHEREAL Aug 15, 2019 •

edited

Loading

IANTHEREAL Aug 15, 2019 •

edited

Loading

IANTHEREAL commented Aug 16, 2019 •

edited

Loading

3pointer commented Aug 25, 2019 •

edited

Loading