pump client: increase retry time, and refine some code #158

WangXiangUSTC · 2019-01-02T09:46:59Z

What problem does this PR solve?

the retry time interval is too small, may cause no available pump error when update the tidb cluster.
issue: https://internal.pingcap.net/jira/browse/TOOL-772

What is changed and how it works?

use write binlog timeout config set in tidb
when all pumps write binlog failed, use all the unavailable pump to try again
handle error for watch etcd
refine log and some code

Check List

Tests

Unit test

WangXiangUSTC · 2019-01-02T10:01:38Z

/run-all-tests

zhouqiang-cl · 2019-01-02T13:01:21Z

/rebuild

IANTHEREAL · 2019-01-03T06:42:47Z

@july2993 PTAL

tidb-binlog/pump_client/client.go

IANTHEREAL · 2019-01-03T07:09:12Z

tidb-binlog/pump_client/client.go

 }

 // setPumpAvaliable set pump's isAvaliable, and modify UnAvaliablePumps or AvaliablePumps.
 func (c *PumpsClient) setPumpAvaliable(pump *PumpStatus, avaliable bool) {
 	pump.IsAvaliable = avaliable
 	if pump.IsAvaliable {
-		err := pump.createGrpcClient(c.Security)


nice modification

tidb-binlog/pump_client/client.go

WangXiangUSTC · 2019-01-04T12:12:44Z

/run-all-tests

WangXiangUSTC · 2019-01-07T12:01:15Z

/run-all-tests

july2993

can you show some of of tidb while all pump is temporary unavailable

pkg/etcd/etcd_test.go

july2993 · 2019-01-09T05:54:27Z

tidb-binlog/pump_client/client.go

 	if err != nil {
 		return nil, errors.Trace(err)
 	}
+
+	if len(newPumpsClient.Pumps.Pumps) == 0 {


*.Pumps.Pumps not good name, hard to know what 's it while reading code

it's hard to modify the name now, because tools relay on tidb, and tidb relay on tools too, this variable is used in tidb's unit test, if change this name, will build failed.

if it's not right, we can spend time to fix it. I suggest to create an issue and fix it later

ok, I will create a issue for it

tidb-binlog/pump_client/client.go

july2993 · 2019-01-09T06:50:01Z

tidb-binlog/pump_client/client.go

-				c.setPumpAvaliable(pump, false)
-				pump = c.Selector.Next(binlog, retryTime/5+1)
-				Logger.Debugf("[pumps client] avaliable pumps: %v, write binlog choose pump %v", c.Pumps.AvaliablePumps, pump)
+			if time.Since(startTime) > c.BinlogWriteTimeout {


BinlogWriteTimeout will be the config of tidb ?

[binlog] # enable to write binlog. enable = false # WriteTimeout specifies how long it will wait for writing binlog to pump. write-timeout = "15s" # If IgnoreError is true, when writting binlog meets error, TiDB would stop writting binlog, # but still provide service. ignore-error = false # use socket file to write binlog, for compatible with kafka version tidb-binlog. binlog-socket = ""

what't the timeout of write rpc? what if just one pump return slowly

rpc timeout also use this config, how about use

if time.Since(startTime) > 2 * c.BinlogWriteTimeout {

july2993 · 2019-01-09T06:56:09Z

tidb-binlog/pump_client/selector.go

 	if len(h.Pumps) == 0 {
+		h.TsMap[binlog.StartTs] = nil


why set this and in RangeSelector too

if no available pump, and then write prewrite binlog, will return nil pump, and tidb will get error, and then write a rollback binlog. the rollback binlog should also choose nil pump.

why write a rollback binlog if you fail to write prewrite binlog

tidb write rollback binlog

it's meaningless to write rollback if you did't write p-binlog.

it's meaningless to write rollback if you did't write p-binlog and better to handle unexpect case ASAP, add some comment to WriteBinlog never return err != nil for rollback&commit, i can't make sure this at first time.

IANTHEREAL · 2019-01-09T07:23:18Z

tidb-binlog/pump_client/pump.go

 }

 // NewPumpStatus returns a new PumpStatus according to node's status.
 func NewPumpStatus(status *node.Status, security *tls.Config) *PumpStatus {
 	pumpStatus := &PumpStatus{}
 	pumpStatus.Status = *status
-	pumpStatus.IsAvaliable = (status.State == node.Online)
+	pumpStatus.security = security

 	if status.State != node.Online {
 		return pumpStatus


need to add ErrNum?

I don't understand definition of pump available, is pump unavailable after pump fail to write binlog?

all use pump.IsUsable() now

errNum default is 0

WangXiangUSTC · 2019-01-09T09:24:32Z

@GregoryIan @july2993 PTAL again

tidb-binlog/pump_client/pump.go

IANTHEREAL · 2019-01-09T09:59:50Z

tidb-binlog/pump_client/pump.go

+	p.Lock()
+	defer p.Unlock()
+
+	atomic.StoreInt64(&p.ErrNum, 0)


also put it into create grpc client?

july2993 · 2019-01-09T10:29:34Z

note, this case may be easy to happen
1, stop all pumps, all pump is update to paused
2, receive WriteBinlog, no pumps available( fall to the so called backoffWriteBinlog case)
3, pump start again, or it have started but you update pumps in Selector before receive WriteBinlog

IANTHEREAL · 2019-01-09T10:35:06Z

tidb-binlog/pump_client/pump.go

+		} else {
+			err := p.createGrpcClient()
+			if err != nil {
+				atomic.AddInt64(&p.ErrNum, 1)


put it into createGrpcClient

july2993 · 2019-01-09T10:48:23Z

add some log at unexpect case like in WriteBinlog fall back to backoffWriteBinlog case

IANTHEREAL · 2019-01-09T10:54:36Z

tidb-binlog/pump_client/client.go

@@ -117,6 +114,8 @@ type PumpsClient struct {

 	// binlog socket file path, for compatible with kafka version pump.
 	binlogSocket string
+


RetryTime of PumpsClient is useless

can't remove now, because tools relay on tidb, and tidb relay on tools, this variable is used in tidb's test, if remove this, will build failed. I will add a comment for this variable.

IANTHEREAL · 2019-01-10T02:12:17Z

LGTM

july2993

LGTM

… strategy config (#223) * pump client: compatible with kafka version tidb-binlog && add unit test (#139) * pump client: write commit binlog will never return error (#148) * pkg watcher: move watcher from tidb-enterprise-tools (#146) * pump client: increase retry time, and refine some code (#158) * pump client: add initial log function (#165) * pump client: support change select pump's strategy (#221)

WangXiangUSTC added 4 commits December 29, 2018 14:04

handle watch etcd error

c10f7c8

add lock

a11a636

minor update

508f94b

refine log

7b8da34

WangXiangUSTC added type/bug-fix priority/important status/PTAL labels Jan 2, 2019

add judgement for pump num

78ffe3d

IANTHEREAL reviewed Jan 3, 2019

View reviewed changes

tidb-binlog/pump_client/client.go Show resolved Hide resolved

IANTHEREAL reviewed Jan 3, 2019

View reviewed changes

tidb-binlog/pump_client/client.go Outdated Show resolved Hide resolved

WangXiangUSTC added status/WIP and removed status/PTAL labels Jan 3, 2019

IANTHEREAL reviewed Jan 3, 2019

View reviewed changes

tidb-binlog/pump_client/client.go Outdated Show resolved Hide resolved

IANTHEREAL reviewed Jan 3, 2019

View reviewed changes

tidb-binlog/pump_client/client.go Show resolved Hide resolved

add revision when watch etcd

0f66320

IANTHEREAL reviewed Jan 3, 2019

View reviewed changes

tidb-binlog/pump_client/client.go Outdated Show resolved Hide resolved

IANTHEREAL reviewed Jan 3, 2019

View reviewed changes

tidb-binlog/pump_client/client.go Show resolved Hide resolved

WangXiangUSTC added 3 commits January 4, 2019 13:21

refine selector

42a1d4d

update lof

a40cff9

minor update

57cc02a

WangXiangUSTC added 2 commits January 6, 2019 21:11

refine err check when write binlog done

6d43935

remove useless code

01642d1

use tidb's timeout config

2571ed6

WangXiangUSTC added the status/PTAL label Jan 8, 2019

july2993 reviewed Jan 9, 2019

View reviewed changes

IANTHEREAL reviewed Jan 9, 2019

View reviewed changes

WangXiangUSTC added 4 commits January 9, 2019 16:01

address comment

a00c5d5

save rootpath

e09ceee

move state = online judgement to pump.IsUsable()

1abd45f

save tsMap when write binlog success in backoff

4310d3e

IANTHEREAL reviewed Jan 9, 2019

View reviewed changes

tidb-binlog/pump_client/pump.go Outdated Show resolved Hide resolved

IANTHEREAL reviewed Jan 9, 2019

View reviewed changes

call selector's feedback function after writebinlog done

9730aa6

IANTHEREAL reviewed Jan 9, 2019

View reviewed changes

WangXiangUSTC added 2 commits January 9, 2019 19:02

address comment

9ed17d9

create grpc conn again if meet unavaliable error

047f58b

IANTHEREAL added status/LGT1 and removed status/PTAL labels Jan 10, 2019

WangXiangUSTC added 2 commits January 10, 2019 10:57

add comment and log

a290c02

make sure every avaliable pump have already try

570ac8f

july2993 reviewed Jan 10, 2019

View reviewed changes

july2993 added status/LGT2 and removed status/LGT1 labels Jan 10, 2019

format code

474475f

WangXiangUSTC merged commit 15b677a into pingcap:master Jan 10, 2019

WangXiangUSTC deleted the xiang/refine_pump_client branch January 10, 2019 04:58

WangXiangUSTC added a commit to WangXiangUSTC/tidb-tools that referenced this pull request Mar 25, 2019

pump client: increase retry time, and refine some code (pingcap#158)

c70015a

WangXiangUSTC mentioned this pull request Mar 25, 2019

pump client: add compatible with kafka version && refine retry && add strategy config #223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pump client: increase retry time, and refine some code #158

pump client: increase retry time, and refine some code #158

WangXiangUSTC commented Jan 2, 2019 •

edited

Loading

WangXiangUSTC commented Jan 2, 2019

zhouqiang-cl commented Jan 2, 2019

IANTHEREAL commented Jan 3, 2019

IANTHEREAL Jan 3, 2019

WangXiangUSTC commented Jan 4, 2019

WangXiangUSTC commented Jan 7, 2019

july2993 left a comment

july2993 Jan 9, 2019

WangXiangUSTC Jan 9, 2019

IANTHEREAL Jan 9, 2019

WangXiangUSTC Jan 10, 2019

july2993 Jan 9, 2019

WangXiangUSTC Jan 9, 2019

july2993 Jan 9, 2019

WangXiangUSTC Jan 9, 2019

july2993 Jan 9, 2019

WangXiangUSTC Jan 9, 2019

july2993 Jan 9, 2019

WangXiangUSTC Jan 9, 2019

july2993 Jan 9, 2019

july2993 Jan 9, 2019

IANTHEREAL Jan 9, 2019

IANTHEREAL Jan 9, 2019

WangXiangUSTC Jan 9, 2019

WangXiangUSTC Jan 9, 2019

WangXiangUSTC commented Jan 9, 2019

IANTHEREAL Jan 9, 2019 •

edited

Loading

july2993 commented Jan 9, 2019

IANTHEREAL Jan 9, 2019

july2993 commented Jan 9, 2019

IANTHEREAL Jan 9, 2019

WangXiangUSTC Jan 9, 2019

IANTHEREAL commented Jan 10, 2019

july2993 left a comment

		@@ -117,6 +114,8 @@ type PumpsClient struct {

		// binlog socket file path, for compatible with kafka version pump.
		binlogSocket string

pump client: increase retry time, and refine some code #158

pump client: increase retry time, and refine some code #158

Conversation

WangXiangUSTC commented Jan 2, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

WangXiangUSTC commented Jan 2, 2019

zhouqiang-cl commented Jan 2, 2019

IANTHEREAL commented Jan 3, 2019

Choose a reason for hiding this comment

WangXiangUSTC commented Jan 4, 2019

WangXiangUSTC commented Jan 7, 2019

july2993 left a 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

WangXiangUSTC commented Jan 9, 2019

IANTHEREAL Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

july2993 commented Jan 9, 2019

Choose a reason for hiding this comment

july2993 commented Jan 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IANTHEREAL commented Jan 10, 2019

july2993 left a comment

Choose a reason for hiding this comment

WangXiangUSTC commented Jan 2, 2019 •

edited

Loading

IANTHEREAL Jan 9, 2019 •

edited

Loading