*: add `Reader`, `Transformer` for relay log #108

csuzhangxc · 2019-04-10T09:40:26Z

What problem does this PR solve?

this PR is one part of #91

What is changed and how it works?

add a MockReader used for testing
add a FileReader used for:
1. read relay log by syncer unit later
2. read relay log for recovering/rollbacking an incomplete relay log file
add a GenRotateEvent to generate RoateEvent for testing
add a Reader (wrap TCPReader) used to read binlog events from master
1. handle some errors
2. do retry if possible
add a Transformer used to transform binlog events
1. extract binlog position/GTID
2. decide the event whether needed to be written to relay log

Check List

Tests

Unit test

for relay log, at least following things needed to do:

add a Writer used to write binlog events to relay log file
compose Reader, Transformer and Writer together to pull binlog events from a master server and write them to files
wrap them to support master-slave switch and clean old code

I'll do them in some PRs later.

the flow of relay log (without master-slave switch) will be:

TCPReaderread binlog events from the master
Reader handle possible errors
Transfomer get events from Reader and transform them
Writer get events from Transformer and write them to disk (with a FileWriter)

… offset status for FileReader

csuzhangxc · 2019-04-12T12:30:40Z

@GregoryIan @amyangfei PTAL

IANTHEREAL · 2019-04-15T05:59:40Z

pkg/binlog/event/event.go

@@ -159,6 +159,33 @@ func GenFormatDescriptionEvent(header *replication.EventHeader, latestPos uint32
 	return ev, errors.Trace(err)
 }

+// GenRotateEvent generates a RotateEvent.
+// ref: https://dev.mysql.com/doc/internals/en/rotate-event.html
+func GenRotateEvent(header *replication.EventHeader, latestPos uint32, nextLogName []byte, position uint64) (*replication.BinlogEvent, error) {


what's difference between latestPos and position?

latestPos is the previous end position of the latest events or the start position of this event.
position is the rotate target position for this event or the start position of the next event.

IANTHEREAL · 2019-04-15T06:17:30Z

pkg/binlog/reader/file.go

+		err := r.parser.ParseFile(pos.Name, int64(pos.Pos), r.onEvent)
+		if err != nil {
+			select {
+			case r.ech <- err:


at least output error information

addressed. log the error before the select.

IANTHEREAL · 2019-04-15T06:27:42Z

pkg/binlog/reader/file.go

+// GetEvent implements Reader.GetEvent.
+func (r *FileReader) GetEvent(ctx context.Context) (*replication.BinlogEvent, error) {
+	if r.stage.Get() != int32(stagePrepared) {
+		return nil, errors.Errorf("stage %s, expect %s", readerStage(r.stage.Get()), stagePrepared)


user/debuger need to know these stage very well, it's better to add a little annotations, like "please start sync"

IANTHEREAL · 2019-04-15T06:29:35Z

pkg/binlog/reader/file.go

+	case <-ctx.Done():
+		return nil, ctx.Err()
+	case <-r.ctx.Done(): // Reader closed
+		return nil, r.ctx.Err()


should add more annotations to distinguish errorr.ctx.Err() with errorctx.Err() at L141

~~added~~ removed r.ctx.Done(). changed the definition. Close will be blocked if GetEvent has not returned (use RWMutex rather than Mutex and AtomicInt32)

IANTHEREAL · 2019-04-15T08:22:18Z

pkg/binlog/reader/mock_test.go

+		}
+	}
+
+	c.Assert(ctx.Err(), IsNil)


it's contradict with || ctx.Err() != nil at L107, can we allow it's timeout?

|| ctx.Err() != nil is used to break the for loop if there are bugs.
if timemout, then the test should be failed in this c.Assert(ctx.Err(), IsNil).
we can put c.Assert(ctx.Err(), IsNil) into the for loop.

IANTHEREAL · 2019-04-15T08:41:42Z

relay/reader/reader.go

+}
+
+// Close implements Reader.Close.
+func (r *reader) Close() error {


lack of lock?

❤️ added. and change Mutex to RWMutex, and remove the atomic variable check

IANTHEREAL · 2019-04-15T08:51:25Z

relay/reader/reader.go

+		ev, err := r.in.GetEvent(ctx)
+		if err == nil {
+			return ev, nil
+		} else if isRetryableError(err) {


if ctx is timeout, it would be infinite loop

removed timeout error handing in this reader, that should be handled in the outer caller.

IANTHEREAL · 2019-04-15T08:54:10Z

relay/reader/reader.go

+	if err != nil {
+		log.Errorf("[relay] start sync for master %s from GTID set %s error %v",
+			r.cfg.MasterID, gs, errors.ErrorStack(err))
+		return r.setUpReaderByPos()


I think we can't try setUpReaderByPos on some errors, like stage is not right. And if we need to try setUpReaderByPos, we should check position to avoid it read binlogs from oldest position 💀

OK, let's put this error to be handled by the outer or even the user.

IANTHEREAL · 2019-04-15T09:24:42Z

relay/transformer/transformer.go

+	Ignore      bool          // whether the event should be ignored
+	LogPos      uint32        // binlog event's End_log_pos or Position in RotateEvent
+	NextLogName string        // next binlog filename, only valid for RotateEvent
+	GTIDSet     mysql.GTIDSet // GTIDSet got from QueryEvent and XIDEvent when RawModeEnabled not true


it's gtid or gtidset?

👍 It's gtid set. It's GTID in one binlog event, but go-mysql returns a GTID set (including the previous/passed in GTID set)

relay/transformer/transformer.go

IANTHEREAL · 2019-04-15T09:38:29Z

relay/transformer/transformer.go

+)
+
+// Result represents a transform result.
+type Result struct {


I don't understand this design very well, like

if some binlog is ignored, should writer write it?

what's its transform rules?

binlog event may be ignored in multiple steps in the processing chain, at least:

some fake/artificial events, which will be marked as ignored in this transformer

some obsolete events (like FormatDescriptionEvent), which will be ignored by the writer

maybe the name of transformer is not accurate, and in fact, more features may be added into this later.

Co-Authored-By: csuzhangxc <csuzhangxc@gmail.com>

csuzhangxc · 2019-04-15T12:39:00Z

/run-all-tests

csuzhangxc · 2019-04-15T12:47:17Z

@GregoryIan PTAL again

pkg/binlog/reader/file.go

pkg/binlog/reader/mock_test.go

relay/reader/reader.go

relay/reader/reader_test.go

Co-Authored-By: csuzhangxc <csuzhangxc@gmail.com>

csuzhangxc · 2019-04-19T03:54:04Z

@GregoryIan @amyangfei PTAL again

pkg/binlog/reader/file_test.go

amyangfei

LGTM

IANTHEREAL · 2019-04-22T04:43:02Z

pkg/binlog/reader/file.go

+		return errors.New("already closed")
+	}
+
+	r.parser.Stop()


it's better to put it after L135, even I know it's ok after I review its implementation

IANTHEREAL · 2019-04-22T05:55:35Z

LGTM

csuzhangxc added 4 commits April 9, 2019 20:32

reader: simple wrapper for TCPReader to reader binlog events from master

81d0b46

reader: add a MockReader

0e7747e

reader: test reader with MockReader

9a6553a

*: test GTID backoff to position

ef76773

csuzhangxc added priority/important Major change, requires approval from ≥2 primary reviewers status/WIP This PR is still work in progress type/enhancement Performance improvement or refactoring labels Apr 10, 2019

csuzhangxc added 10 commits April 10, 2019 19:25

reader: get event handle retryable and ignorable error

9fa15ad

reader: add test cases for error

33e22e0

event: add GenRotateEvent

1f5bc1e

*: add a transformer

76ba02c

reader: add a FileReader

0500128

reader: add channel buffer size control for FileReader; add read/send…

2f9db5b

… offset status for FileReader

reader: rename struct

86a3967

reader: tiny modify FileReader; add test case for FileReader

af31c57

Merge remote-tracking branch 'remotes/origin/master' into relay-writer

de5dbf6

reader: tiny fix; add more test cases for FileReader

6bde450

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 Apr 12, 2019

csuzhangxc changed the title *: refactor relay writer framework *: add Reader, Transformer for relay log Apr 12, 2019

IANTHEREAL reviewed Apr 15, 2019

View reviewed changes

csuzhangxc and others added 7 commits April 15, 2019 18:24

reader: DeadlineExceeded should be retry in the outer caller

93dd964

reader: address comments

1be78cb

reader: address comments

83fc175

reader: address comments

43a7808

Update relay/transformer/transformer.go

59a6a7b

Co-Authored-By: csuzhangxc <csuzhangxc@gmail.com>

reader: change Mutex and Atomic to RWMutex

d0b6697

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

ef1f976

*: refine code

208aa36

amyangfei reviewed Apr 18, 2019

View reviewed changes

amyangfei and others added 5 commits April 18, 2019 11:01

Update pkg/binlog/reader/file.go

1184ffa

Co-Authored-By: csuzhangxc <csuzhangxc@gmail.com>

Update relay/reader/reader.go

ab3418c

Co-Authored-By: csuzhangxc <csuzhangxc@gmail.com>

*: address comments

5edf31b

reader: refine error msg

b8c006a

*: refine code

a5e98b7

reader: add FileReaderStatus string representation

521ad40

amyangfei reviewed Apr 19, 2019

View reviewed changes

pkg/binlog/reader/file_test.go Outdated Show resolved Hide resolved

reader: address comment

0e318d1

amyangfei reviewed Apr 22, 2019

View reviewed changes

amyangfei added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Apr 22, 2019

IANTHEREAL reviewed Apr 22, 2019

View reviewed changes

reader: address comment

3f146d0

IANTHEREAL added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Apr 22, 2019

csuzhangxc merged commit c452c25 into pingcap:master Apr 22, 2019

csuzhangxc deleted the relay-writer branch April 22, 2019 06:05

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

*: add Reader, Transformer for relay log (pingcap#108)

afef51a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add `Reader`, `Transformer` for relay log #108

*: add `Reader`, `Transformer` for relay log #108

csuzhangxc commented Apr 10, 2019 •

edited

Loading

csuzhangxc commented Apr 12, 2019

IANTHEREAL Apr 15, 2019

csuzhangxc Apr 15, 2019

IANTHEREAL Apr 15, 2019

csuzhangxc Apr 15, 2019

IANTHEREAL Apr 15, 2019

IANTHEREAL Apr 15, 2019

csuzhangxc Apr 15, 2019 •

edited

Loading

IANTHEREAL Apr 15, 2019

csuzhangxc Apr 15, 2019 •

edited

Loading

IANTHEREAL Apr 15, 2019

csuzhangxc Apr 15, 2019

IANTHEREAL Apr 15, 2019

csuzhangxc Apr 15, 2019

IANTHEREAL Apr 15, 2019

csuzhangxc Apr 15, 2019 •

edited

Loading

IANTHEREAL Apr 15, 2019

csuzhangxc Apr 15, 2019

IANTHEREAL Apr 15, 2019

csuzhangxc Apr 15, 2019

csuzhangxc commented Apr 15, 2019

csuzhangxc commented Apr 15, 2019

csuzhangxc commented Apr 19, 2019

amyangfei left a comment

IANTHEREAL Apr 22, 2019 •

edited

Loading

csuzhangxc Apr 22, 2019

IANTHEREAL commented Apr 22, 2019

*: add Reader, Transformer for relay log #108

*: add Reader, Transformer for relay log #108

Conversation

csuzhangxc commented Apr 10, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

csuzhangxc commented Apr 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csuzhangxc Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csuzhangxc Apr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csuzhangxc Apr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csuzhangxc commented Apr 15, 2019

csuzhangxc commented Apr 15, 2019

csuzhangxc commented Apr 19, 2019

amyangfei left a comment

Choose a reason for hiding this comment

IANTHEREAL Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IANTHEREAL commented Apr 22, 2019

*: add `Reader`, `Transformer` for relay log #108

*: add `Reader`, `Transformer` for relay log #108

csuzhangxc commented Apr 10, 2019 •

edited

Loading

csuzhangxc Apr 15, 2019 •

edited

Loading

csuzhangxc Apr 15, 2019 •

edited

Loading

csuzhangxc Apr 15, 2019 •

edited

Loading

IANTHEREAL Apr 22, 2019 •

edited

Loading