dm-worker: refine conn #266

3pointer · 2019-09-02T11:50:36Z

What problem does this PR solve?

Optimize Database connections in dm-worker units, after unify DB operations

What is changed and how it works?

one worker use one individual connection
same functional connection comes from one DB object
define some package for DB object and connections, all DB object generate in conn.DBProvider.Apply
split upstream DB connection and downstream(worker) DB connection, because they have totally different DB operations, it's better use *sql.DB in upstream, and use individual connection for each worker in downstream.
define two basic object, BaseDB and BaseConn,
BaseDB generates BaseConn, BaseConn wraps sql.Conn with own retry policy
BaseDB -> BaseConn correspond to sql.DB -> sql.Conn
In our scenario, there are two main reasons why we need BaseConn
1. we often need one fixed DB connection to execute sql
2. we need own retry policy during execute failed
  So we split a fixed sql.Conn out of sql.DB, and wraps it to BaseConn
  And Similar with sql.Conn, all BaseConn generated from one BaseDB shares this BaseDB to reset

Check List

Tests

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

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

Need to cherry-pick to the release branch
Need to update the documentation
Need to update the dm/dm-ansible
Need to be included in the release note

codecov · 2019-09-02T11:57:46Z

Codecov Report

Merging #266 into master will decrease coverage by 0.1888%.
The diff coverage is 21.8487%.

@@              Coverage Diff               @@
##             master     #266        +/-   ##
==============================================
- Coverage   60.1288%   59.94%   -0.1889%     
==============================================
  Files           136      135         -1     
  Lines         16448    15010      -1438     
==============================================
- Hits           9890     8997       -893     
+ Misses         5625     5141       -484     
+ Partials        933      872        -61

csuzhangxc · 2019-09-02T13:44:32Z

@amyangfei @WangXiangUSTC PTAL

syncer/checkpoint.go

dm/config/subtask.go

loader/checkpoint.go

loader/db.go

loader/loader.go

pkg/conn/baseconn.go

loader/db.go

pkg/conn/basedb.go

syncer/db.go

syncer/syncer.go

WangXiangUSTC · 2019-09-06T08:01:08Z

loader has struct Conn, syncer has struct WorkerConn, and they both have function createConn, can we unify them as a pkg?

3pointer · 2019-09-09T02:58:11Z

loader has struct Conn, syncer has struct WorkerConn, and they both have function createConn, can we unify them as a pkg?

loader and syncer although have similar conn struct, but they also have different log content and stats content, so they are not same struct, if we unify their createConn into one pkg, we should return either same struct or same interface, then how to deal with different log and stats?

split workerConn and upstreamConn in syncer split conn and db unify conn in syncer and loader

3pointer · 2019-09-16T06:07:29Z

syncer/syncer.go

@@ -920,11 +916,6 @@ func (s *Syncer) sync(ctx *tcontext.Context, queueBucket string, db *Conn, jobCh
 			idx++

 			if sqlJob.tp == ddl {
-				err = executeSQLs()


this block of code is useless, so I removed it, @amyangfei please check it

loader/checkpoint.go

loader/loader.go

syncer/db.go

csuzhangxc · 2019-10-16T02:32:31Z

@WangXiangUSTC @amyangfei PTAL

amyangfei · 2019-10-17T10:11:52Z

please resolve the conflicts

3pointer · 2019-10-17T12:41:13Z

/run-all-tests

syncer/syncer.go

loader/loader.go

pkg/conn/baseconn.go

amyangfei

LGTM

sre-bot · 2019-10-18T07:31:52Z

cherry pick to release-1.0 failed

* define conn split workerConn and upstreamConn in syncer split conn and db unify conn in syncer and loader * change upstream DB to BaseDB * update raw db config

3pointer added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/enhancement Performance improvement or refactoring labels Sep 2, 2019

csuzhangxc requested review from amyangfei and WangXiangUSTC September 2, 2019 13:44

WangXiangUSTC reviewed Sep 3, 2019

View reviewed changes

3pointer force-pushed the refine_conn branch from 41a0855 to e0c0234 Compare September 3, 2019 12:11

amyangfei reviewed Sep 4, 2019

View reviewed changes

pkg/conn/baseconn.go Show resolved Hide resolved

3pointer force-pushed the refine_conn branch 2 times, most recently from d31cf58 to 22d5987 Compare September 4, 2019 12:51

WangXiangUSTC reviewed Sep 5, 2019

View reviewed changes

loader/db.go Show resolved Hide resolved

WangXiangUSTC reviewed Sep 5, 2019

View reviewed changes

loader/db.go Outdated Show resolved Hide resolved

WangXiangUSTC reviewed Sep 5, 2019

View reviewed changes

pkg/conn/basedb.go Outdated Show resolved Hide resolved

pkg/conn/basedb.go Outdated Show resolved Hide resolved

syncer/db.go Outdated Show resolved Hide resolved

syncer/syncer.go Outdated Show resolved Hide resolved

3pointer force-pushed the refine_conn branch from f6cc483 to 10d7d99 Compare September 9, 2019 02:59

3pointer added the needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated label Sep 9, 2019

define conn

f8ece5b

split workerConn and upstreamConn in syncer split conn and db unify conn in syncer and loader

3pointer force-pushed the refine_conn branch from 645f501 to f8ece5b Compare September 16, 2019 04:56

3pointer added needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 and removed needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Sep 16, 2019

3pointer added 4 commits September 16, 2019 13:01

change upstream DB to BaseDB

ba9c588

update raw db config

25b7016

fix test

7ce2dac

remove useless code

ecf7bf5

3pointer commented Sep 16, 2019

View reviewed changes

add resetConnFn instead of baseDB in syncer.WorkerConn to reset

f579445

amyangfei reviewed Sep 17, 2019

View reviewed changes

csuzhangxc added the status/LGT1 One reviewer already commented LGTM label Oct 16, 2019

3pointer added 2 commits October 16, 2019 11:47

return commit err index

b8afd4d

resolve conflict

ae9080c

csuzhangxc added the priority/release-blocker This PR blocks a release. Please review it ASAP. label Oct 16, 2019

3pointer added 2 commits October 17, 2019 13:09

Merge branch 'master' into refine_conn

1104960

fix test

0a62846

csuzhangxc mentioned this pull request Oct 17, 2019

mydumper: generate tables needed to dump for mydumper automatically #310

Merged

resolve conflict

6407d25

amyangfei reviewed Oct 18, 2019

View reviewed changes

syncer/syncer.go Outdated Show resolved Hide resolved

WangXiangUSTC reviewed Oct 18, 2019

View reviewed changes

loader/loader.go Outdated Show resolved Hide resolved

pkg/conn/baseconn.go Outdated Show resolved Hide resolved

3pointer added 2 commits October 18, 2019 13:09

address comments

8ac2bf1

address comment

aaa8665

amyangfei reviewed Oct 18, 2019

View reviewed changes

amyangfei added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Oct 18, 2019

WangXiangUSTC approved these changes Oct 18, 2019

View reviewed changes

3pointer merged commit 26b90b3 into pingcap:master Oct 18, 2019

csuzhangxc added status/LGT3 Three reviewers already commented LGTM, ready for merge and removed status/LGT2 Two reviewers already commented LGTM, ready for merge labels Oct 18, 2019

3pointer deleted the refine_conn branch October 18, 2019 07:31

3pointer added a commit to 3pointer/dm that referenced this pull request Oct 18, 2019

dm-worker: refine conn (pingcap#266)

7049c77

* define conn split workerConn and upstreamConn in syncer split conn and db unify conn in syncer and loader * change upstream DB to BaseDB * update raw db config

3pointer mentioned this pull request Oct 18, 2019

dm-worker: refine conn (#266) #325

Merged

IANTHEREAL pushed a commit that referenced this pull request Oct 19, 2019

dm-worker: refine conn (#266) (#325)

f4cf10e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dm-worker: refine conn #266

dm-worker: refine conn #266

3pointer commented Sep 2, 2019 •

edited

Loading

codecov bot commented Sep 2, 2019 •

edited

Loading

csuzhangxc commented Sep 2, 2019

WangXiangUSTC commented Sep 6, 2019

3pointer commented Sep 9, 2019

3pointer Sep 16, 2019

csuzhangxc commented Oct 16, 2019

amyangfei commented Oct 17, 2019

3pointer commented Oct 17, 2019

amyangfei left a comment

sre-bot commented Oct 18, 2019

dm-worker: refine conn #266

dm-worker: refine conn #266

Conversation

3pointer commented Sep 2, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Sep 2, 2019 • edited Loading

Codecov Report

csuzhangxc commented Sep 2, 2019

WangXiangUSTC commented Sep 6, 2019

3pointer commented Sep 9, 2019

3pointer Sep 16, 2019

Choose a reason for hiding this comment

csuzhangxc commented Oct 16, 2019

amyangfei commented Oct 17, 2019

3pointer commented Oct 17, 2019

amyangfei left a comment

Choose a reason for hiding this comment

sre-bot commented Oct 18, 2019

3pointer commented Sep 2, 2019 •

edited

Loading

codecov bot commented Sep 2, 2019 •

edited

Loading