Adding replication protocol support to mysql server implementation #759
Conversation
feat: method in BinlogStreamer, work with replication protocol in server package
feat: all methods for replication protocol
|
Hi @Fizic is this PR still a draft? |
| return &p, nil | ||
| } | ||
|
|
||
| func parseBinlogDumpGTID(data []byte) (*mysql.MysqlGTIDSet, error) { |
There was a problem hiding this comment.
do you plan to support MariaDB GTID set? you can see ParseMariadbGTID
There was a problem hiding this comment.
MariaDB doesn't implement COM_BINLOG_DUMP_GTID - so, we can assume that this message sent by MySQL.
https://github.com/MariaDB/server/blob/10.9/include/mysql_com.h#L118
There was a problem hiding this comment.
Yes, I mean if there's not too much work to do we can support MariaDB by the way. But as you said MariaDB does not use COM_BINLOG_DUMP_GTID so it may not be a trivial work
server/command.go
Outdated
| s := replication.NewBinlogStreamer() | ||
| go h.HandleBinlogDump(pos, s) | ||
|
|
||
| return s |
There was a problem hiding this comment.
another choice is that this Conn never returns, we directly call writeBinlogEvents in a loop. I'm not sure what should we do if the Conn is returned and MySQL client send another command to this connection
There was a problem hiding this comment.
Interesting question. mysql-clients can be disconnected when they are not responsive (see net_write_timeout and net_read_timeout). We should check how replication protocol works.
There was a problem hiding this comment.
Thanks for reminding us, there're two questions
- I wrongly thought the connection will be re-used for next command at first, and want to remind author to check what's the behaviour if the connection is both serving a binlog stream and client's request. Now I see
writeBinlogEventswon't do that. - mysql client may set heartbeat interval and binlog streamer should append heartbeat event when the stream is idle for the interval. I think it's OK to implement it later because some use case of this feature will not let stream be idle for a long time.
server/command.go
Outdated
| //handle Replication command | ||
| HandleRegisterSlave(data []byte) error | ||
| HandleBinlogDump(pos *Position, s *replication.BinlogStreamer) | ||
| HandleBinlogDumpGTID(gtidSet *MysqlGTIDSet, s *replication.BinlogStreamer) |
There was a problem hiding this comment.
I think that s *replication.BinlogStreamer shouldn't be here. It is up to users who will implement ReplicationHandler what to do with these events.
PS: feel free to disagree
There was a problem hiding this comment.
Sorry I don't understand, you mean we can expose Conn to user and user directly write binlog event as []byte to Conn?
There was a problem hiding this comment.
Oh... I worded it really badly.
I will agree with you that it is not a good idea to expose Conn.
There was a problem hiding this comment.
For consistency we can
- move
s *replication.BinlogStreamerto return values - add
errorto return values - remove
goinConn.dispatch()... or document that this method will be called form its own coroutine.
| } | ||
|
|
||
| func (h EmptyReplicationHandler) HandleBinlogDumpGTID(gtidSet *MysqlGTIDSet, r *replication.BinlogStreamer) { | ||
| } |
There was a problem hiding this comment.
I suggest adding r.close() here, just to gracefully release resources.
server/command.go
Outdated
| } | ||
| return nil | ||
| } else { | ||
| return fmt.Errorf("the handler does not support replication protocol, use ReplicationHandler instead") |
There was a problem hiding this comment.
Maybe it is better to fall back to
return c.h.HandleOtherCommand(cmd, data)
… could lead to errors feat: comments explaining options, ReplicationHandler in tests
There was a problem hiding this comment.
rest lgtm
and I remember mysqlbinlog can actively stop a binlog stream https://dev.mysql.com/doc/refman/8.0/en/mysqlbinlog.html#option_mysqlbinlog_stop-never I'm not sure the termination is done at client-side or server-side so need we implement more behaviour. We can do it later.
|
👍 Looks good to me |
No description provided.