Support MySQL replication named channels (phases 1-3)#78
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds multi-source replication support Phase 1–3: new DB table for per-channel metadata, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DiscoveryEngine as Discovery Engine
participant Instance
participant Database as DB
Client->>DiscoveryEngine: Discover instance
DiscoveryEngine->>Instance: Execute SHOW SLAVE STATUS
Instance-->>DiscoveryEngine: Multi-row result (channels)
DiscoveryEngine->>DiscoveryEngine: Parse rows → ReplicationChannels[]
DiscoveryEngine->>DiscoveryEngine: selectCanonicalChannelIndex()
DiscoveryEngine->>Instance: Set ReplicationChannels, ManagedChannelName
DiscoveryEngine->>Database: writeInstanceChannels()
Database-->>DiscoveryEngine: persisted rows
Note over Client: Later: channel-aware operations
Client->>DiscoveryEngine: Stop/Start/ChangeMaster (may include channel)
DiscoveryEngine->>DiscoveryEngine: determine effectiveChannel
DiscoveryEngine->>Instance: Execute channel-specific SQL (e.g., STOP REPLICA FOR CHANNEL)
Instance-->>DiscoveryEngine: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Phase 1 of issue #77: Support MySQL replication named channels. - Add ChannelStatus struct to represent per-channel replication state (IO/SQL thread status, coordinates, lag, errors, GTID usage) - Add ReplicationChannels and ManagedChannelName fields to Instance struct - Add database_instance_channels table migration for persisting channel data
Phase 2 of issue #77: Parse all SHOW REPLICA STATUS rows into ChannelStatus structs, select a canonical channel for backward compatibility, and persist/read channel data to/from the backend DB. - Parse Channel_Name column from each SHOW SLAVE STATUS row - Build ChannelStatus per row and store in instance.ReplicationChannels - selectCanonicalChannelIndex picks default/non-GR channel for Instance fields - writeInstanceChannels persists channel data after discovery - readInstanceChannels loads channels when reading from backend - Single-source replication behavior is unchanged
Phase 3 of issue #77: Make stop/start replication and change master operations channel-aware for named replication channels. - Add FOR CHANNEL clause helpers to QueryStringProvider for all replication commands (stop, start, reset, IO/SQL thread control) - Add StopReplicationForChannel and StartReplicationForChannel that target a specific channel or fall back to the managed channel - Add ChangeMasterToForChannel with FOR CHANNEL clause on all CHANGE MASTER TO variants (GTID, binlog pos, MariaDB, Oracle) - Original StopReplication/StartReplication/ChangeMasterTo delegate to the new functions with empty channel for backward compatibility
e5e833c to
8f0d92c
Compare
There was a problem hiding this comment.
Code Review
This pull request implements multi-source replication support by introducing a new database table for channel status, updating the Instance model, and adding channel-aware replication control functions. The review feedback highlights a potential SQL syntax error due to unescaped channel names, an N+1 query performance issue when loading instances, and the need for consistent logic and refactoring when determining the effective replication channel across different operations.
I am having trouble creating individual review comments. Click here to see my feedback.
go/inst/query_string_provider.go (402)
The channel name is concatenated directly into the SQL string using single quotes. If a channel name contains a single quote (which is technically possible in MySQL), this will result in a SQL syntax error. It is safer to escape the channel name using strings.ReplaceAll(channelName, "'", "''") (note: this requires importing the strings package).
go/inst/instance_topology_dao.go (1008)
Unlike StopReplicationForChannel and StartReplicationForChannel, this function does not automatically target the ManagedChannelName when channelName is empty on a multi-source instance. This inconsistency may cause ChangeMasterTo (which calls this with an empty channel) to target the wrong channel (the default one) instead of the one orchestrator is supposed to manage.
// For multi-source instances with no explicit channel, use the managed channel
effectiveChannel := channelName
if effectiveChannel == "" && len(instance.ReplicationChannels) > 1 {
effectiveChannel = instance.ManagedChannelName
}
// Build the FOR CHANNEL suffix for multi-source replication
channelClause := forChannelClause(effectiveChannel)go/inst/instance_dao.go (1560)
Executing a separate query for replication channels for every instance row read will lead to an N+1 query problem, significantly impacting performance when reading large clusters or many instances (e.g., for the dashboard or API). Consider fetching all channels for the set of instances in a single query and distributing them to the instances in bulk.
go/inst/instance_topology_dao.go (418-421)
This logic for determining the effective channel name is duplicated in StartReplicationForChannel and should also be applied to ChangeMasterToForChannel. Consider refactoring this into a helper method on the Instance struct (e.g., func (this *Instance) GetEffectiveChannel(channelName string) string).
go/inst/instance_topology_dao.go (1088)
Update the log message to use the effective channel name determined at the beginning of the function.
log.Infof("ChangeMasterTo: Changed master on %+v to: %+v, %+v. GTID: %+v, Channel: %q", *instanceKey, masterKey, masterBinlogCoordinates, changedViaGTID, effectiveChannel)
Summary
Implements phases 1-3 of #77: Support MySQL replication named channels (multi-source replication).
ChannelStatusstruct for per-channel replication state, extendInstancewithReplicationChannelsandManagedChannelNamefields, adddatabase_instance_channelstable migration.SHOW REPLICA STATUSrows intoChannelStatusstructs, select a canonical channel for backward-compatibleInstancefields, persist and read channel data to/from the backend database.FOR CHANNELclause support to all replication SQL commands, addStopReplicationForChannel/StartReplicationForChannel/ChangeMasterToForChannelfunctions that target specific channels.All changes are backward compatible: single-source replication (the common case) behaves identically to before. Group Replication internal channels (
group_replication_applier,group_replication_recovery) are skipped when selecting the canonical channel.Phases 4-6 (visualization, failover, testing) will follow in subsequent PRs.
Closes #77
Test plan
go buildsucceedsgo test ./go/inst/...passesgo test ./go/...passes (all packages)database_instance_channelstable is created on schema migrationSummary by CodeRabbit