EXPLORATION: refactor BinlogStreamer onto go-mysql canal (findings)

[driv3r]

Warning

This is an exploration spike, not a merge candidate. It compiles and is functionally correct on the happy path, but has a hard blocker at cutover (see below). Opening as draft to preserve the work and share findings.

Goal

Evaluate replacing Ghostferry's hand-rolled binlog event loop with go-mysql/canal, keeping the DMLEvent output boundary and GTID coordinate semantics unchanged. The hypothesis (from prior discussion) was that canal would improve maintainability by removing low-level code paths we currently own.

Base branch: gtid-stage5-replica-progress (canal pairs naturally with the GTID work).

What changed

• binlog_streamer.go now drives canal instead of a raw replication.BinlogSyncer. Replaced by a single canal.EventHandler adapter (OnRotate / OnRowsQueryEvent / OnRow / OnGTID / OnXID / OnDDL / OnPosSynced):
  • the manual Run() GetEvent loop
  • the big event-type switch (defaultEventHandler)
  • rotate / format-description / position bookkeeping
  • handleRowsEvent raw decoding
• dml_events.go: row constructors take [][]interface{} (shared), and new NewBinlogDMLEventsFromCanal builds events from canal.RowsEvent (Action-based, no replication event-type switch).
• API change: removed the raw AddBinlogEventHandler / BinlogEventState hooks (only real use was intercepting DDL QueryEvents) in favor of a typed DDLEventHandler func(schema, table string, query []byte) error. ddl_ghostferry harness + streamer tests migrated accordingly.

Simplification measured

Modest.

Metric	Before	After
binlog_streamer.go	802 lines	753 lines
new transitive deps	—	BurntSushi/toml, pingcap/failpoint, tidb parser

The removed code is the gnarly low-level loop, but the DMLEvent construction, coordinate stamping, filtering, stop/cutover, lag metrics, and server-id logic all remain (canal doesn't own those). Net win is real but smaller than hoped, and it adds dependency surface.

🚧 Blocker: canal stop-path deadlock at cutover

Functionally the refactor is correct — unit tests pass, and happy-path copy passes in both file_position and gtid modes. But there is a hard stop-path deadlock at cutover, reproduced consistently at ~50% failure on TestCopyDataWith*:

• canal owns the event loop and is stopped via canal.Close().
• canal.Close() waits on the syncer goroutine (wg.Wait()).
• that goroutine is frequently parked in a blocking socket read — especially the low-traffic target-verifier streamer once the source goes read_only at cutover.
• canal's 100ms close read-deadline does not reliably preempt the in-progress netpoll read, so Close() (and thus Ferry shutdown via StopTargetVerifier) hangs → test timeout → cascade (leftover read_only = ON then fails the next run's replica precheck).

Representative goroutine dump at the hang:

goroutine [sync.WaitGroup.Wait]:
  ghostferry.(*Ferry).StopTargetVerifier
  ...
goroutine [select]:
  ghostferry.(*BinlogStreamer).Run.func2   // stop monitor, inside canal.Close()
goroutine [IO wait]:
  net.(*conn).Read
  go-mysql/packet.(*Conn).ReadPacket
  go-mysql/replication.(*BinlogSyncer).onStream   // parked read, never interrupted

Mitigations attempted (all still flaky):

• cfg.ReadTimeout at 1s and 5s
• cfg.HeartbeatPeriod = 1s
• sync.Once-guarded Close() to avoid double-close
• async Close() from the monitor + defer — made it worse (Run returns mid-teardown, reconnect races)

Conclusion / recommendation

Canal is a genuine maintainability improvement for the event-decode path, but its cooperative-shutdown model is incompatible with Ghostferry's stop-at-coordinate cutover without upstream changes — e.g. a context-cancellable GetEvent, or a non-blocking / force-close Close(). Not mergeable as-is.

This matches the earlier assessment: canal helps maintainability, but the hard parts (here, deterministic stop/cutover) still need Ghostferry-level orchestration — and in this case canal actively fights it.

Possible follow-ups (if we want to pursue canal)

1. Upstream a context-cancellable GetEvent / non-blocking Close() to go-mysql, or
2. Bypass canal.Close() by holding the syncer reference and force-closing the underlying net.Conn, or
3. Keep the raw BinlogSyncer (current approach) and only adopt canal's RowsEvent/Action normalization for the decode layer.

Test status

• ✅ package unit tests (go test .) — both modes
• ✅ TestBinlogStreamerTestSuite (incl. new DDL-handler test) — file_position
• ✅ happy-path copy (TestCopyDataWithInsertLoad) — file_position and gtid
• ⚠️ TestCopyDataWith* under load — ~50% flaky due to the cutover deadlock above

[driv3r]

@Shopify/db-mobility @milanatshopify @pawandubey - just as an FYI for the future