feat: add MySQL 8.4-specific replication and group replication templates (#60)#62
Conversation
Add dedicated templates for MySQL 8.4+ that eliminate deprecation warnings from removed/renamed variables: - repl_crash_safe_options84.gotxt: omits master-info-repository and relay-log-info-repository (removed in 8.4), keeps relay-log-recovery=on - group_repl_options84.gotxt: omits transaction_write_set_extraction (removed in 8.4) and uses log_replica_updates unconditionally - init_slaves_84.gotxt: documents 8.4+ CHANGE REPLICATION SOURCE TO syntax - init_nodes84.gotxt: group replication init using 8.4+ replication commands Update replication.go to use SOURCE_AUTO_POSITION instead of MASTER_AUTO_POSITION for MySQL 8.0.23+, and select the appropriate init_slaves template based on version. Update group_replication.go, multi-source-replication.go, pxc_replication.go, and cmd/single.go to select the 8.4-specific crash-safe and group options templates when the MySQL version is >= 8.4.0.
📝 WalkthroughWalkthroughAdds MySQL 8.4-specific replication and connection templates to support syntax changes in MySQL 8.4. Introduces new template constants, template files, and version-aware selection logic across single, group, master-slave, multi-source, and PXC replication topologies. When sandbox version >= Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces support for MySQL 8.4+ by implementing version-aware template selection for single, group, and multi-source replication. It addresses the removal of deprecated parameters like master-info-repository and transaction_write_set_extraction while introducing new syntax such as SOURCE_AUTO_POSITION. The review feedback highlights several opportunities to improve efficiency and maintainability by refactoring duplicated version checks and moving constant checks outside of loops.
| sd.ReplCrashSafeOptions = sandbox.SingleTemplates[globals.TmplReplCrashSafeOptions].Contents | ||
| // Use 8.4+ crash-safe options template (no deprecated master-info-repository) | ||
| crashSafeTmpl := globals.TmplReplCrashSafeOptions | ||
| isMySQL84, _ := common.GreaterOrEqualVersion(sd.Version, globals.MinimumResetBinaryLogsVersion) |
There was a problem hiding this comment.
This version check and the subsequent template selection logic (lines 428-432) are identical to the block at lines 404-408. Consider refactoring this logic into a common variable or a helper function to improve maintainability and avoid duplication.
| useReplicaUpdates, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumShowReplicaStatusVersion) | ||
| useNoWriteSetExtraction, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumNoWriteSetExtractionVersion) | ||
| // Use 8.4+ group replication options template when applicable | ||
| useMySQL84GroupOptions, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) |
There was a problem hiding this comment.
This version check is redundant because it was already performed at line 293 (isMySQL84). Additionally, performing this check inside the loop is inefficient as sandboxDef.Version remains constant across iterations.
| useMySQL84GroupOptions, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) | |
| useMySQL84GroupOptions := isMySQL84 |
|
|
||
| // Select the appropriate init_slaves template based on MySQL version | ||
| initSlavesTemplate := globals.TmplInitSlaves | ||
| useNewSourceSyntax, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumChangeReplicationSourceVersion) |
There was a problem hiding this comment.
This version check is redundant as it was already performed at line 227 (useSourceAutoPosition). Reusing the existing variable improves readability and efficiency.
| useNewSourceSyntax, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumChangeReplicationSourceVersion) | |
| useNewSourceSyntax := useSourceAutoPosition |
| // master-info-repository and relay-log-info-repository removed in 8.4+ | ||
| skipCrashSafeOpts, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) | ||
| if !skipCrashSafeOpts { | ||
| isMySQL84pxc, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) |
There was a problem hiding this comment.
This version check is performed inside a loop (starting at line 249), but sandboxDef.Version is constant. Moving this check outside the loop would be more efficient.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sandbox/templates/replication/init_slaves_84.gotxt (1)
28-31: Inconsistent indentation and shell convention for exit code comparison.Two minor issues:
- Line 30 lacks indentation within the
ifblock, making the script harder to read.- The exit code comparison uses string equality (
"0") instead of numeric comparison (-eq 0), which is unconventional in shell scripts.♻️ Suggested fix for formatting and convention
- if [ "$exit_code" == "0" ] + if [ "$exit_code" -eq 0 ] then -rm -f ./post_initialization + rm -f ./post_initialization fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sandbox/templates/replication/init_slaves_84.gotxt` around lines 28 - 31, The if-statement comparing exit_code is using string equality and the rm command is not indented; update the conditional in the block that references the variable exit_code to use numeric comparison (e.g., -eq 0) instead of "==" "0", and indent the rm -f ./post_initialization line inside the if body (the block controlling removal of post_initialization) so the script follows standard shell conventions and consistent formatting.sandbox/group_replication.go (1)
348-365: Consider reusing theisMySQL84variable computed earlier.The version check at line 349 (
useMySQL84GroupOptions) duplicates the check at line 293 (isMySQL84). Consider reusingisMySQL84to avoid redundant version comparisons.♻️ Suggested consolidation
- // Use 8.4+ group replication options template when applicable - useMySQL84GroupOptions, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumResetBinaryLogsVersion) ... groupReplOptionsTmpl := globals.TmplGroupReplOptions - if useMySQL84GroupOptions { + if isMySQL84 { groupReplOptionsTmpl = globals.TmplGroupReplOptions84 } ... - if useMySQL84GroupOptions { + if isMySQL84 { // relay-log-recovery is still valid; use the 8.4-specific template🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sandbox/group_replication.go` around lines 348 - 365, The code duplicates a version check by computing useMySQL84GroupOptions instead of reusing the earlier isMySQL84; replace the second check with the existing isMySQL84 variable (remove the call to common.GreaterOrEqualVersion for sandboxDef.Version) and use isMySQL84 when selecting groupReplOptionsTmpl (affecting variables useMySQL84GroupOptions, groupReplOptionsTmpl and the branch that sets TmplGroupReplOptions84) so the template selection uses the previously computed isMySQL84 boolean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sandbox/repl_templates.go`:
- Around line 141-145: The template globals.TmplInitSlaves84 is described as
MySQL 8.4+ but the selection logic in replication.go currently uses
MinimumChangeReplicationSourceVersion (8.0.23); update that selection to use
MinimumResetBinaryLogsVersion (8.4.0) instead so TmplInitSlaves84 is only
applied for MySQL versions that support the 8.4+ syntax (mirror the approach
used for TmplInitNodes84 in sandbox/group_replication.go).
In `@sandbox/templates/group/init_nodes84.gotxt`:
- Line 13: The template hardcodes replication credentials ('rsandbox') in the
user_cmd string; update the construction of user_cmd (the line building user_cmd
that uses .ChangeMasterTo, .MasterUserParam, .MasterPasswordParam,
.ChangeMasterExtra and FOR CHANNEL 'group_replication_recovery') to inject the
configured replication username and password fields (e.g. .RplUser and
.RplPassword) instead of the literal 'rsandbox', preserving existing
quoting/escaping and placement of .MasterUserParam/.MasterPasswordParam so
customized RplUser/RplPassword values are used during group replication init.
---
Nitpick comments:
In `@sandbox/group_replication.go`:
- Around line 348-365: The code duplicates a version check by computing
useMySQL84GroupOptions instead of reusing the earlier isMySQL84; replace the
second check with the existing isMySQL84 variable (remove the call to
common.GreaterOrEqualVersion for sandboxDef.Version) and use isMySQL84 when
selecting groupReplOptionsTmpl (affecting variables useMySQL84GroupOptions,
groupReplOptionsTmpl and the branch that sets TmplGroupReplOptions84) so the
template selection uses the previously computed isMySQL84 boolean.
In `@sandbox/templates/replication/init_slaves_84.gotxt`:
- Around line 28-31: The if-statement comparing exit_code is using string
equality and the rm command is not indented; update the conditional in the block
that references the variable exit_code to use numeric comparison (e.g., -eq 0)
instead of "==" "0", and indent the rm -f ./post_initialization line inside the
if body (the block controlling removal of post_initialization) so the script
follows standard shell conventions and consistent formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf18a2cd-2e0f-4938-8093-18375d45ffaf
📒 Files selected for processing (13)
cmd/single.goglobals/template_names.gosandbox/group_replication.gosandbox/group_templates.gosandbox/multi-source-replication.gosandbox/pxc_replication.gosandbox/repl_templates.gosandbox/replication.gosandbox/templates.gosandbox/templates/group/group_repl_options84.gotxtsandbox/templates/group/init_nodes84.gotxtsandbox/templates/replication/init_slaves_84.gotxtsandbox/templates/single/repl_crash_safe_options84.gotxt
| globals.TmplInitSlaves84: TemplateDesc{ | ||
| Description: "Initialize slaves after deployment (MySQL 8.4+ syntax)", | ||
| Notes: "Uses CHANGE REPLICATION SOURCE TO and SOURCE_AUTO_POSITION", | ||
| Contents: initSlaves84Template, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify version constant values and usage consistency
echo "=== Version constant definitions ==="
rg -n 'Minimum(ChangeReplicationSource|ResetBinaryLogs)Version' --type=go -A1
echo ""
echo "=== Usage in template selection ==="
rg -n 'TmplInitSlaves84|TmplInitNodes84' --type=go -B3 -A1Repository: ProxySQL/dbdeployer
Length of output: 4849
Fix version threshold for template selection to match 8.4+ syntax requirement.
The template is named and described for MySQL 8.4+ syntax, but is selected using MinimumChangeReplicationSourceVersion (8.0.23) instead of MinimumResetBinaryLogsVersion (8.4.0). This inconsistency conflicts with the "MySQL 8.4+ syntax" description and differs from the parallel TmplInitNodes84 template in sandbox/group_replication.go, which correctly uses MinimumResetBinaryLogsVersion for 8.4-specific features.
Update sandbox/replication.go (around line 508) to select TmplInitSlaves84 using MinimumResetBinaryLogsVersion instead, ensuring the template is only applied to MySQL versions that actually support its declared 8.4+ syntax.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sandbox/repl_templates.go` around lines 141 - 145, The template
globals.TmplInitSlaves84 is described as MySQL 8.4+ but the selection logic in
replication.go currently uses MinimumChangeReplicationSourceVersion (8.0.23);
update that selection to use MinimumResetBinaryLogsVersion (8.4.0) instead so
TmplInitSlaves84 is only applied for MySQL versions that support the 8.4+ syntax
(mirror the approach used for TmplInitNodes84 in sandbox/group_replication.go).
| [ -z "$SLEEP_TIME" ] && SLEEP_TIME=1 | ||
| {{range .Nodes}} | ||
| user_cmd='{{.ResetMasterCmd}};' | ||
| user_cmd="$user_cmd {{.ChangeMasterTo}} {{.MasterUserParam}}='rsandbox', {{.MasterPasswordParam}}='rsandbox' {{.ChangeMasterExtra}} FOR CHANNEL 'group_replication_recovery';" |
There was a problem hiding this comment.
Hardcoded replication credentials ignore configured values.
Line 13 always uses 'rsandbox' credentials, which can fail group replication init when RplUser/RplPassword are customized.
Suggested fix
- user_cmd="$user_cmd {{.ChangeMasterTo}} {{.MasterUserParam}}='rsandbox', {{.MasterPasswordParam}}='rsandbox' {{.ChangeMasterExtra}} FOR CHANNEL 'group_replication_recovery';"
+ user_cmd="$user_cmd {{.ChangeMasterTo}} {{.MasterUserParam}}='{{.RplUser}}', {{.MasterPasswordParam}}='{{.RplPassword}}' {{.ChangeMasterExtra}} FOR CHANNEL 'group_replication_recovery';"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| user_cmd="$user_cmd {{.ChangeMasterTo}} {{.MasterUserParam}}='rsandbox', {{.MasterPasswordParam}}='rsandbox' {{.ChangeMasterExtra}} FOR CHANNEL 'group_replication_recovery';" | |
| user_cmd="$user_cmd {{.ChangeMasterTo}} {{.MasterUserParam}}='{{.RplUser}}', {{.MasterPasswordParam}}='{{.RplPassword}}' {{.ChangeMasterExtra}} FOR CHANNEL 'group_replication_recovery';" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sandbox/templates/group/init_nodes84.gotxt` at line 13, The template
hardcodes replication credentials ('rsandbox') in the user_cmd string; update
the construction of user_cmd (the line building user_cmd that uses
.ChangeMasterTo, .MasterUserParam, .MasterPasswordParam, .ChangeMasterExtra and
FOR CHANNEL 'group_replication_recovery') to inject the configured replication
username and password fields (e.g. .RplUser and .RplPassword) instead of the
literal 'rsandbox', preserving existing quoting/escaping and placement of
.MasterUserParam/.MasterPasswordParam so customized RplUser/RplPassword values
are used during group replication init.
There was a problem hiding this comment.
Pull request overview
Adds MySQL 8.4+ template variants for replication and group replication so deployments can use newer syntax/variables and avoid deprecation warnings, while keeping older MySQL versions on existing templates.
Changes:
- Introduces new 8.4+ templates for crash-safe replication options, slave init, group init, and group replication options.
- Embeds/registers new templates and adds version-based selection in replication/group/PXC/multi-source/single flows.
- Updates GTID auto-position option selection to use
SOURCE_AUTO_POSITIONstarting with MySQL 8.0.23.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sandbox/templates/single/repl_crash_safe_options84.gotxt | New crash-safe options template for MySQL 8.4+ (drops removed vars). |
| sandbox/templates/replication/init_slaves_84.gotxt | New slave init script template labeled for 8.4+ syntax. |
| sandbox/templates/group/init_nodes84.gotxt | New group replication init script template for 8.4+. |
| sandbox/templates/group/group_repl_options84.gotxt | New group replication options template for 8.4+ variable set. |
| sandbox/templates.go | Embeds/registers repl_crash_safe_options84. |
| sandbox/replication.go | Adds SOURCE_AUTO_POSITION gating and selects init-slaves template by version. |
| sandbox/repl_templates.go | Embeds/registers init_slaves_84. |
| sandbox/pxc_replication.go | Uses 8.4 crash-safe options template for PXC at 8.4+. |
| sandbox/multi-source-replication.go | Uses 8.4 crash-safe options template for fan-in/all-masters at 8.4+. |
| sandbox/group_templates.go | Embeds/registers group init/options 8.4 templates. |
| sandbox/group_replication.go | Selects 8.4+ group init/options/crash-safe templates by version. |
| globals/template_names.go | Adds new template name constants for the 8.4 variants. |
| cmd/single.go | Selects 8.4 crash-safe options template for single sandboxes when applicable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useSourceAutoPosition, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumChangeReplicationSourceVersion) | ||
| if useSourceAutoPosition { | ||
| masterAutoPosition = ", SOURCE_AUTO_POSITION=1" | ||
| logger.Printf("Adding SOURCE_AUTO_POSITION to slaves setup\n") | ||
| } else { | ||
| masterAutoPosition = ", MASTER_AUTO_POSITION=1" | ||
| logger.Printf("Adding MASTER_AUTO_POSITION to slaves setup\n") | ||
| } |
There was a problem hiding this comment.
New code uses common.GreaterOrEqualVersion(), which is marked DEPRECATED (common/checks.go notes to use GreaterOrEqualVersionList + flavors or common.HasCapability). Consider switching this version gate to the non-deprecated helpers to avoid adding more deprecated call sites (and handle/propagate the comparison error instead of discarding it).
| // Select the appropriate init_slaves template based on MySQL version | ||
| initSlavesTemplate := globals.TmplInitSlaves | ||
| useNewSourceSyntax, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumChangeReplicationSourceVersion) | ||
| if useNewSourceSyntax { |
There was a problem hiding this comment.
The init-slaves template selection uses MinimumChangeReplicationSourceVersion (8.0.23) to choose globals.TmplInitSlaves84. Either the template/name should reflect that it targets 8.0.23+ (not 8.4+), or the selection should use the 8.4 gate if the intent is truly “84+”. Leaving this as-is is confusing and makes future maintenance harder.
| // Select the appropriate init_slaves template based on MySQL version | |
| initSlavesTemplate := globals.TmplInitSlaves | |
| useNewSourceSyntax, _ := common.GreaterOrEqualVersion(sandboxDef.Version, globals.MinimumChangeReplicationSourceVersion) | |
| if useNewSourceSyntax { | |
| // Select the appropriate init_slaves template based on MySQL version. | |
| // TmplInitSlaves84 is intended for MySQL 8.4+; earlier versions use the legacy template. | |
| initSlavesTemplate := globals.TmplInitSlaves | |
| useInitSlaves84, _ := common.GreaterOrEqualVersion(sandboxDef.Version, "8.4.0") | |
| if useInitSlaves84 { |
|
|
||
| # Don't use directly. | ||
| # This script is called by 'start_all' when needed | ||
| # Uses MySQL 8.4+ replication syntax (CHANGE REPLICATION SOURCE TO) |
There was a problem hiding this comment.
This header says the script is for “MySQL 8.4+”, but in code it’s selected starting at 8.0.23 (MinimumChangeReplicationSourceVersion). Please update the comment (and/or the template name) so it matches the actual version range where it’s used.
| # Uses MySQL 8.4+ replication syntax (CHANGE REPLICATION SOURCE TO) | |
| # Uses MySQL 8.0.23+ replication syntax (CHANGE REPLICATION SOURCE TO) |
| TmplInitNodes84 = "init_nodes84" | ||
| TmplGroupReplOptions84 = "group_repl_options84" | ||
|
|
||
| // MySQL 8.4+ specific templates |
There was a problem hiding this comment.
The “MySQL 8.4+ specific templates” section includes TmplInitSlaves84, but current selection logic uses it for >= 8.0.23. Please adjust the comment/grouping (or rename the template) so this list doesn’t misrepresent the version boundary.
| // MySQL 8.4+ specific templates | |
| // MySQL 8.0.23+ specific templates |
| // Use 8.4+ crash-safe options template (no deprecated master-info-repository) | ||
| crashSafeTmpl := globals.TmplReplCrashSafeOptions | ||
| isMySQL84, _ := common.GreaterOrEqualVersion(sd.Version, globals.MinimumResetBinaryLogsVersion) | ||
| if isMySQL84 { | ||
| crashSafeTmpl = globals.TmplReplCrashSafeOptions84 | ||
| } | ||
| sd.ReplCrashSafeOptions = sandbox.SingleTemplates[crashSafeTmpl].Contents |
There was a problem hiding this comment.
This adds a new common.GreaterOrEqualVersion() call to gate 8.4-specific behavior. Since GreaterOrEqualVersion is deprecated, prefer common.HasCapability (if an appropriate capability exists) or VersionToList + GreaterOrEqualVersionList, and avoid ignoring the returned error.
| // Use 8.4+ crash-safe options template (no deprecated master-info-repository) | ||
| crashSafeTmpl := globals.TmplReplCrashSafeOptions | ||
| isMySQL84, _ := common.GreaterOrEqualVersion(sd.Version, globals.MinimumResetBinaryLogsVersion) | ||
| if isMySQL84 { | ||
| crashSafeTmpl = globals.TmplReplCrashSafeOptions84 | ||
| } | ||
| sd.ReplCrashSafeOptions = sandbox.SingleTemplates[crashSafeTmpl].Contents |
There was a problem hiding this comment.
Same as above: this introduces another common.GreaterOrEqualVersion() call (deprecated) for 8.4 gating. Please use the non-deprecated helpers and handle the comparison error instead of discarding it.
Summary
Dedicated templates for MySQL 8.4+ that use the modern replication syntax, eliminating deprecation warnings.
New templates:
repl_crash_safe_options84.gotxt— omits removed variables (master-info-repository,relay-log-info-repository)init_slaves_84.gotxt— usesCHANGE REPLICATION SOURCE TOsyntaxgroup_repl_options84.gotxt— dropstransaction_write_set_extraction(removed in 8.4), useslog_replica_updates=ONinit_nodes84.gotxt— group replication init with 8.4+ variable namesVersion-based selection:
SOURCE_AUTO_POSITIONOlder MySQL versions (5.7, 8.0) continue using original templates unchanged.
Closes #60
Test Plan
Summary by CodeRabbit