relay: retry for upstream bad connection #265

csuzhangxc · 2019-09-02T09:44:45Z

What problem does this PR solve?

retry to read binlog events from upstream when encountering bad connection error

What is changed and how it works?

check return error from binlog reader
use pkg/backoff to trigger the retry

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)
- run DM-worker with fetching binlog events
- KILL connection for the relay unit
- observe whether relay retry to fetch binlog events again

Side effects

Increased code complexity

…24f4b6f38e3e4ed1037a1a05acd GO111MODULE=on go mod tidy

codecov · 2019-09-02T12:51:40Z

Codecov Report

Merging #265 into master will decrease coverage by 0.2887%.
The diff coverage is 88.3116%.

@@               Coverage Diff                @@
##             master       #265        +/-   ##
================================================
- Coverage   60.2806%   59.9918%   -0.2888%     
================================================
  Files           134        134                
  Lines         14965      14742       -223     
================================================
- Hits           9021       8844       -177     
+ Misses         5099       5052        -47     
- Partials        845        846         +1

csuzhangxc · 2019-09-02T13:47:59Z

/run-all-tests

csuzhangxc · 2019-09-02T14:30:15Z

/run-all-tests

csuzhangxc · 2019-09-03T03:02:50Z

@GregoryIan @amyangfei PTAL

relay/relay.go

IANTHEREAL · 2019-09-03T05:22:46Z

pkg/parser/common_test.go

@@ -83,11 +83,7 @@ func (t *testParserSuite) TestParser(c *C) {
 	}

 	unsupportedSQLs := []string{
-		"alter table bar ADD FULLTEXT INDEX `fulltext` (`name`) WITH PARSER ngram",


remove two supported cases handy?

YES, because parser also updated when executing

GO111MODULE=on go get -u github.com/siddontang/go-mysql@f52d30c9fcb7824f4b6f38e3e4ed1037a1a05acd

relay/relay.go

IANTHEREAL · 2019-09-03T06:08:25Z

relay/retry/reader.go

+		return false
+	}
+
+	for lrt := rr.lastRetryTime; time.Since(lrt) > rr.cfg.BackoffRollback; lrt = lrt.Add(rr.cfg.BackoffRollback) {


can we this logic code into backoff pkg? but not now

and a function like func (b *Backoff) Adjust() to adjust/rollback cwnd?
If so, maybe too many things backoff can do?
what your opinion? @amyangfei

I prefer not to add this logic into backoff pkg, but maybe we can add a function like RollbackN?

IANTHEREAL · 2019-09-03T06:09:49Z

relay/relay.go

+		}
+
+		r.tctx.L().Info("receive retryable error for binlog reader", log.ShortError(err))
+		err = reader2.Close() // close the previous reader


can we close reader2 twice?

YES, we can close Reader any times, but it will return a can not close error except the first one.

In this place, we only close a reader2 instance once. this L298 close the previous one, and in the defer in L260 ~ L265, it may close the first one (without retry) or the latest one (with retry).

check reader2 != nil in c6baaab.

IANTHEREAL · 2019-09-03T06:23:13Z

Rest LGTM

IANTHEREAL · 2019-09-03T07:05:50Z

LGTM

amyangfei · 2019-09-03T07:17:03Z

dm/worker/relay.go

@@ -96,6 +97,13 @@ func NewRealRelayHolder(cfg *Config) RelayHolder {
 		},
 		BinLogName: clone.RelayBinLogName,
 		BinlogGTID: clone.RelayBinlogGTID,
+		ReaderRetry: rr.ReaderRetryConfig{ // we use config for TaskChecker now


Suggested change

ReaderRetry: rr.ReaderRetryConfig{ // we use config for TaskChecker now

ReaderRetry: rr.ReaderRetryConfig{ // we use config from TaskChecker now

amyangfei · 2019-09-03T07:17:32Z

dm/worker/relay.go

@@ -320,6 +328,13 @@ func (h *realRelayHolder) Update(ctx context.Context, cfg *Config) error {
 			User:     cfg.From.User,
 			Password: cfg.From.Password,
 		},
+		ReaderRetry: rr.ReaderRetryConfig{ // we use config for TaskChecker now


amyangfei

LGTM

csuzhangxc added 4 commits September 2, 2019 11:51

relay: only update GTID for DDL query (and XID)

e856e28

GO111MODULE=on go get -u github.com/siddontang/go-mysql@f52d30c9fcb78…

a176f37

…24f4b6f38e3e4ed1037a1a05acd GO111MODULE=on go mod tidy

relay: add a backoff retry mechanism

5cd63b4

*: relay retry for upstream bad connection

73a038d

csuzhangxc added 2 commits September 2, 2019 18:00

*: fix CI

e12cb98

*: fix CI

31c7bbf

csuzhangxc added 2 commits September 2, 2019 21:34

relay: add test case for relay retry

5afa82f

relay: fix CI

be219a2

amyangfei added the priority/release-blocker This PR blocks a release. Please review it ASAP. label Sep 3, 2019

relay: try fix CI

fc7149b

csuzhangxc 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 Sep 3, 2019

csuzhangxc requested review from amyangfei and IANTHEREAL September 3, 2019 03:02

amyangfei reviewed Sep 3, 2019

View reviewed changes

relay/relay.go Outdated Show resolved Hide resolved

csuzhangxc and others added 2 commits September 3, 2019 11:44

relay: address comment

6f8de21

Merge branch 'master' into relay-retry

8c7cc99

IANTHEREAL reviewed Sep 3, 2019

View reviewed changes

relay/relay.go Show resolved Hide resolved

IANTHEREAL reviewed Sep 3, 2019

View reviewed changes

Merge branch 'master' into relay-retry

f0282f5

csuzhangxc added 2 commits September 3, 2019 14:37

relay: address comment

c6baaab

Merge remote-tracking branch 'origin/relay-retry' into relay-retry

37d6bc7

amyangfei reviewed Sep 3, 2019

View reviewed changes

relay: address comments

73688c0

amyangfei approved these changes Sep 3, 2019

View reviewed changes

amyangfei added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Sep 3, 2019

csuzhangxc merged commit 14eae68 into pingcap:master Sep 3, 2019

csuzhangxc deleted the relay-retry branch September 3, 2019 07:45

lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020

relay: retry for upstream bad connection (pingcap#265)

e6aba71

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

relay: retry for upstream bad connection #265

relay: retry for upstream bad connection #265

csuzhangxc commented Sep 2, 2019 •

edited

Loading

codecov bot commented Sep 2, 2019 •

edited

Loading

csuzhangxc commented Sep 2, 2019

csuzhangxc commented Sep 2, 2019

csuzhangxc commented Sep 3, 2019

IANTHEREAL Sep 3, 2019

csuzhangxc Sep 3, 2019

IANTHEREAL Sep 3, 2019

csuzhangxc Sep 3, 2019

amyangfei Sep 3, 2019 •

edited

Loading

IANTHEREAL Sep 3, 2019

csuzhangxc Sep 3, 2019

csuzhangxc Sep 3, 2019

IANTHEREAL commented Sep 3, 2019

IANTHEREAL commented Sep 3, 2019

amyangfei Sep 3, 2019

amyangfei Sep 3, 2019

amyangfei left a comment

	ReaderRetry: rr.ReaderRetryConfig{ // we use config for TaskChecker now
	ReaderRetry: rr.ReaderRetryConfig{ // we use config from TaskChecker now

relay: retry for upstream bad connection #265

relay: retry for upstream bad connection #265

Conversation

csuzhangxc 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

csuzhangxc commented Sep 2, 2019

csuzhangxc commented Sep 3, 2019

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

amyangfei Sep 3, 2019 • 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

IANTHEREAL commented Sep 3, 2019

IANTHEREAL commented Sep 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amyangfei left a comment

Choose a reason for hiding this comment

csuzhangxc commented Sep 2, 2019 •

edited

Loading

codecov bot commented Sep 2, 2019 •

edited

Loading

amyangfei Sep 3, 2019 •

edited

Loading