Fix group replication and multi-source for MySQL 8.4+#50
Conversation
Critical fixes: - Remove transaction_write_set_extraction for 8.3+ (removed variable) - Skip master-info-repository and relay-log-info-repository for 8.4+ (removed) - Use RESET BINARY LOGS AND GTIDS instead of RESET MASTER for 8.4+ Deprecated option fixes: - Use log_replica_updates instead of log_slave_updates for 8.0.22+ - Version-aware in group replication, PXC, and multi-source templates Tested with MySQL 8.4.4 and 9.1.0 — all 3 group replication nodes ONLINE. Closes #49
|
Warning Rate limit exceeded
⌛ 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 (9)
✨ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves several compatibility issues related to group replication and multi-source replication in MySQL 8.4+ and 9.x. It adapts replication commands and options based on the MySQL version, addressing critical changes and deprecated features. The changes ensure that replication setups function correctly on newer MySQL versions while maintaining compatibility with older versions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses compatibility issues with MySQL 8.3+ and 8.4+ by making several options and commands version-conditional. The changes are well-implemented across different replication setups, and the refactoring to simplify template data population is a good improvement. I have a few minor suggestions to enhance code clarity and maintainability.
| skipCrashSafe2, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) | ||
| if !skipCrashSafe2 { | ||
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents | ||
| } |
There was a problem hiding this comment.
The variable name skipCrashSafe2 is a bit confusing. Since this variable is local to the CreateFanInReplication function, you could reuse the name skipCrashSafeOpts as used in CreateAllMastersReplication for consistency.
| skipCrashSafe2, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) | |
| if !skipCrashSafe2 { | |
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents | |
| } | |
| skipCrashSafeOpts, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) | |
| if !skipCrashSafeOpts { | |
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents | |
| } |
| useReplicaUpdates, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumShowReplicaStatusVersion) | ||
| if useReplicaUpdates { | ||
| sandboxDef.ReplOptions = fmt.Sprintf("%s\nlog_replica_updates=ON\n", sandboxDef.ReplOptions) | ||
| } else { | ||
| sandboxDef.ReplOptions = fmt.Sprintf("%s\nlog_slave_updates=ON\n", sandboxDef.ReplOptions) | ||
| } |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, you can determine the option name first and then use it in a single fmt.Sprintf call.
| useReplicaUpdates, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumShowReplicaStatusVersion) | |
| if useReplicaUpdates { | |
| sandboxDef.ReplOptions = fmt.Sprintf("%s\nlog_replica_updates=ON\n", sandboxDef.ReplOptions) | |
| } else { | |
| sandboxDef.ReplOptions = fmt.Sprintf("%s\nlog_slave_updates=ON\n", sandboxDef.ReplOptions) | |
| } | |
| useReplicaUpdates, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumShowReplicaStatusVersion) | |
| optionName := "log_slave_updates" | |
| if useReplicaUpdates { | |
| optionName = "log_replica_updates" | |
| } | |
| sandboxDef.ReplOptions = fmt.Sprintf("%s\n%s=ON\n", sandboxDef.ReplOptions, optionName) |
| minimum_version_reset_binary_logs="008004000" | ||
| if [[ "v$sortable_version" > "v$minimum_version_reset_binary_logs" ]] || [[ "v$sortable_version" == "v$minimum_version_reset_binary_logs" ]]; then | ||
| ./use -e 'RESET BINARY LOGS AND GTIDS' | ||
| else | ||
| ./use -e 'reset master' | ||
| fi |
There was a problem hiding this comment.
This logic for choosing the reset master command can be improved for maintainability and readability.
- The hardcoded version
008004000could become out of sync if the corresponding Go constant changes. It would be better to pass this value from the Go code into the template. - The shell condition
[[... > ...]] || [[... == ...]]can be simplified to>=.
Since changing the Go code to pass the version is out of scope for this file, I'll suggest simplifying the shell logic for now.
minimum_version_reset_binary_logs="008004000"
if [[ "v$sortable_version" >= "v$minimum_version_reset_binary_logs" ]]; then
./use -e 'RESET BINARY LOGS AND GTIDS'
else
./use -e 'reset master'
fi
There was a problem hiding this comment.
Pull request overview
Updates dbdeployer’s replication/group-replication deployment logic and templates to handle MySQL 8.4+ / 9.x incompatibilities (removed variables and removed SQL commands), avoiding startup failures and reducing deprecation warnings.
Changes:
- Introduces version cutoffs for MySQL 8.3+ (skip
transaction_write_set_extraction) and 8.4+ (useRESET BINARY LOGS AND GTIDS, skip crash-safe repository options). - Adds
ResetMasterCmdinto replication command maps and templates to centralize the 8.4+ reset behavior. - Makes group-replication and PXC replication options version-aware (
log_replica_updatesvslog_slave_updates) and skips removed crash-safe options.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| globals/globals.go | Adds new version constants for 8.3+ and 8.4+ gating. |
| sandbox/replication.go | Adds ResetMasterCmd to replication command mapping and selects command based on version. |
| sandbox/group_replication.go | Passes ResetMasterCmd to templates; version-conditions group replication options and crash-safe options. |
| sandbox/pxc_replication.go | Switches to log_replica_updates for newer versions; skips crash-safe options for 8.4+. |
| sandbox/multi-source-replication.go | Skips crash-safe options for 8.4+ and simplifies population of template command variables. |
| sandbox/templates/single/clear.gotxt | Replaces RESET MASTER with version-conditional reset logic for 8.4+. |
| sandbox/templates/replication/multi_source.gotxt | Uses {{.ResetMasterCmd}} instead of hardcoded reset master. |
| sandbox/templates/group/init_nodes.gotxt | Uses {{.ResetMasterCmd}} when initializing nodes. |
| sandbox/templates/group/group_repl_options.gotxt | Makes log_replica_updates and transaction_write_set_extraction conditional by version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents | ||
| skipCrashSafeOpts, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) | ||
| if !skipCrashSafeOpts { | ||
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents |
There was a problem hiding this comment.
When skipCrashSafeOpts is true (8.4+), sandboxDef.ReplCrashSafeOptions is left untouched. Since SandboxDef can already carry crash-safe options (e.g., set by --gtid / --repl-crash-safe via fillSandboxDefinition), this can accidentally keep master-info-repository/relay-log-info-repository in the generated my.cnf and still break 8.4+ startup. Explicitly set sandboxDef.ReplCrashSafeOptions = "" in the skip path to avoid leaking preexisting options.
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents | |
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents | |
| } else { | |
| // For versions >= MinimumResetBinaryLogsVersion (e.g. 8.4+), ensure that any | |
| // pre-existing crash-safe replication options are cleared so they are not | |
| // written into my.cnf where they could break server startup. | |
| sandboxDef.ReplCrashSafeOptions = "" |
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents | ||
| skipCrashSafe2, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) | ||
| if !skipCrashSafe2 { | ||
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents |
There was a problem hiding this comment.
Same issue as above: when skipCrashSafe2 is true (8.4+), sandboxDef.ReplCrashSafeOptions is not cleared, so any pre-populated crash-safe options may still be emitted into configs and trigger 8.4+ startup failures. Set sandboxDef.ReplCrashSafeOptions = "" in the skip branch to ensure removed options aren’t carried over.
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents | |
| sandboxDef.ReplCrashSafeOptions = SingleTemplates[globals.TmplReplCrashSafeOptions].Contents | |
| } else { | |
| sandboxDef.ReplCrashSafeOptions = "" |
| useResetBinaryLogs, _ := common.GreaterOrEqualVersion(version, globals.MinimumResetBinaryLogsVersion) | ||
| if useResetBinaryLogs { | ||
| cmds["ResetMasterCmd"] = "RESET BINARY LOGS AND GTIDS" | ||
| } |
There was a problem hiding this comment.
This introduces a new version gate using common.GreaterOrEqualVersion, which is marked DEPRECATED in common/checks.go and returns false for MariaDB 10+ regardless of the comparison target. Also the returned error is ignored, so any unexpected version format would silently fall back to reset master (which is removed in MySQL 8.4+). Consider switching this new check to VersionToList + GreaterOrEqualVersionList (handling the error) or a flavor-based capability check, so ResetMasterCmd selection can’t silently regress.
| // master-info-repository and relay-log-info-repository removed in 8.4+ | ||
| skipCrashSafeOpts, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) | ||
| if !skipCrashSafeOpts { | ||
| sandboxDef.ReplOptions += fmt.Sprintf("\n%s\n", SingleTemplates[globals.TmplReplCrashSafeOptions].Contents) | ||
| } |
There was a problem hiding this comment.
MinimumResetBinaryLogsVersion is being used here as the cutoff for skipping crash-safe replication options (because master-info-repository/relay-log-info-repository were removed in 8.4+). Since the constant name describes a different behavior (reset binary logs), this coupling makes future maintenance harder (e.g., if one cutoff changes independently). Consider introducing a dedicated version constant for the removal of crash-safe repository options, and use that here.
| // master-info-repository and relay-log-info-repository removed in 8.4+ | ||
| skipCrashSafeOpts, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) | ||
| if !skipCrashSafeOpts { | ||
| sandboxDef.ReplOptions += fmt.Sprintf("\n%s\n", SingleTemplates[globals.TmplReplCrashSafeOptions].Contents) | ||
| } |
There was a problem hiding this comment.
Same maintainability concern: this uses MinimumResetBinaryLogsVersion as a proxy for when crash-safe repository options were removed. If these version thresholds diverge later, it’s easy for this check to become incorrect. Consider a dedicated constant for the crash-safe option removal threshold and use it here.
| minimum_version_reset_binary_logs="008004000" | ||
| if [[ "v$sortable_version" > "v$minimum_version_reset_binary_logs" ]] || [[ "v$sortable_version" == "v$minimum_version_reset_binary_logs" ]]; then | ||
| ./use -e 'RESET BINARY LOGS AND GTIDS' | ||
| else | ||
| ./use -e 'reset master' | ||
| fi |
There was a problem hiding this comment.
sortable_version is computed only inside the earlier if [ -n "$(is_running)" ] block, but it’s used here to choose between RESET BINARY LOGS AND GTIDS and reset master. If the sandbox isn’t running (or is_running returns empty), sortable_version will be unset and this version check can select the wrong command (notably falling back to reset master on 8.4+). Consider initializing VERSION/sortable_version unconditionally near the top of the script (or computing it again in this block) before doing the comparison.
Summary
Fixes group replication deployment on MySQL 8.4+ and 9.x.
Critical (caused startup failure)
transaction_write_set_extraction=XXHASH64removed in 8.3+ — now version-conditionalmaster-info-repository=table/relay-log-info-repository=tableremoved in 8.4+ — skipped for 8.4+RESET MASTERremoved in 8.4+ — replaced withRESET BINARY LOGS AND GTIDSDeprecated (warnings)
log_slave_updates→log_replica_updatesfor 8.0.22+Files changed
globals/globals.go— new version constantssandbox/replication.go—ResetMasterCmdin replication commandssandbox/group_replication.go— version-aware options + crash-safe skipsandbox/pxc_replication.go— version-aware log_replica_updates + crash-safe skipsandbox/multi-source-replication.go— simplified replCmds loop + crash-safe skipTested
Closes #49