Replace deprecated rpl_semi_sync_master/slave with rpl_semi_sync_sour…#892
Replace deprecated rpl_semi_sync_master/slave with rpl_semi_sync_sour…#892shunki-fujita wants to merge 1 commit intomainfrom
Conversation
shunki-fujita
force-pushed
the
issue-847
branch
from
0e71e45 to
7431096
Compare
…ce/replica Migrate semi-synchronous replication from the deprecated master/slave plugins to the new source/replica plugins for MySQL 9.x compatibility. Key changes: - Add SemiSyncNames type and DetectSemiSyncNames() to dynamically detect which semi-sync plugin is installed via information_schema.plugins - Make all semi-sync variable references dynamic (SET GLOBAL, SELECT) based on the detected plugin type - Rename struct fields: SemiSyncMasterEnabled -> SemiSyncSourceEnabled, SemiSyncSlaveEnabled -> SemiSyncReplicaEnabled, WaitForSlaveCount -> WaitForReplicaCount - Update test helpers to install new plugin names Existing clusters with legacy plugins continue to work because the controller detects the installed plugin at runtime. Migration happens when moco-agent re-initializes plugins on Pod restart. Ref: #847 Signed-off-by: shunki-fujita <shunki-fujita@cybozu.co.jp>
shunki-fujita
force-pushed
the
issue-847
branch
from
7431096 to
37fc649
Compare
There was a problem hiding this comment.
Pull request overview
This pull request migrates MOCO's semi-synchronous replication from deprecated MySQL master/slave plugin terminology to the new source/replica terminology for MySQL 9.x compatibility. The changes introduce runtime plugin detection to maintain backward compatibility with existing clusters using legacy plugins while supporting new installations with modern plugin names.
Changes:
- Added
SemiSyncNamestype andDetectSemiSyncNames()function to dynamically detect installed semi-sync plugin variant - Refactored all semi-sync variable references to use dynamic naming based on detected plugin type
- Renamed struct fields to use source/replica terminology (e.g.,
SemiSyncMasterEnabled→SemiSyncSourceEnabled)
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/dbop/semisync.go | New file implementing plugin detection logic and name mappings for both legacy and new plugin variants |
| pkg/dbop/types.go | Renamed GlobalVariables and GlobalStatus struct fields to source/replica terminology with aliased db tags |
| pkg/dbop/operator.go | Integrated plugin detection during operator initialization with fallback to new names on error |
| pkg/dbop/status.go | Refactored queries to dynamically build SQL using detected plugin variable names |
| pkg/dbop/replication.go | Updated ConfigureReplica and ConfigurePrimary to use dynamic variable names via fmt.Sprintf |
| pkg/dbop/test_util.go | Updated test helper to install new plugin names and added detection call in test operator creation |
| pkg/dbop/status_test.go | Updated test assertions to use new struct field names |
| pkg/dbop/replication_test.go | Updated test assertions to use new struct field names |
| clustering/status.go | Updated to use renamed GlobalStatus field |
| clustering/operations.go | Updated to use renamed GlobalVariables fields |
| clustering/mock_test.go | Updated mock implementation to use new field names |
| clustering/manager_test.go | Updated test assertions to use new field names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Detect semi-sync plugin type. If the instance is unreachable (e.g. during failover), | ||
| // default to new names. Subsequent queries will also fail and be handled gracefully. | ||
| ss, _ := DetectSemiSyncNames(ctx, db) |
There was a problem hiding this comment.
Silently ignoring the error from DetectSemiSyncNames could mask real database connectivity issues during operator initialization. While the comment mentions this is intentional for failover scenarios, this creates an inconsistent state where we may use wrong plugin names even when the database is actually reachable but has a different issue (e.g., permission problems querying information_schema.plugins). Consider logging the error at a warning level to aid debugging, especially since subsequent queries will fail anyway.
| func DetectSemiSyncNames(ctx context.Context, db *sqlx.DB) (*SemiSyncNames, error) { | ||
| // Check for new plugin first | ||
| status, err := getPluginStatus(ctx, db, NewSemiSyncNames.SourcePluginName) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to check semi-sync source plugin: %w", err) | ||
| } | ||
| if status == "ACTIVE" { | ||
| return &NewSemiSyncNames, nil | ||
| } | ||
|
|
||
| // Check for legacy plugin | ||
| status, err = getPluginStatus(ctx, db, LegacySemiSyncNames.SourcePluginName) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to check semi-sync master plugin: %w", err) | ||
| } | ||
| if status == "ACTIVE" { | ||
| return &LegacySemiSyncNames, nil | ||
| } | ||
|
|
||
| // Neither installed — assume new names for fresh install | ||
| return &NewSemiSyncNames, nil |
There was a problem hiding this comment.
The detection logic checks if the plugin status is "ACTIVE", but MySQL plugins can be in other states like "INACTIVE", "DISABLED", or "DELETED". If a plugin is installed but not ACTIVE (e.g., INACTIVE or DISABLED), the function will fall through and potentially return the wrong name set. Consider checking if the plugin exists (status != "") rather than only checking if it's ACTIVE, or explicitly handling other states. This could cause issues if a plugin is installed but disabled.
| func DetectSemiSyncNames(ctx context.Context, db *sqlx.DB) (*SemiSyncNames, error) { | ||
| // Check for new plugin first | ||
| status, err := getPluginStatus(ctx, db, NewSemiSyncNames.SourcePluginName) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to check semi-sync source plugin: %w", err) | ||
| } | ||
| if status == "ACTIVE" { | ||
| return &NewSemiSyncNames, nil | ||
| } | ||
|
|
||
| // Check for legacy plugin | ||
| status, err = getPluginStatus(ctx, db, LegacySemiSyncNames.SourcePluginName) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to check semi-sync master plugin: %w", err) | ||
| } | ||
| if status == "ACTIVE" { | ||
| return &LegacySemiSyncNames, nil | ||
| } | ||
|
|
||
| // Neither installed — assume new names for fresh install | ||
| return &NewSemiSyncNames, nil | ||
| } |
There was a problem hiding this comment.
The DetectSemiSyncNames function lacks unit test coverage. Given its critical role in determining which MySQL plugin naming convention to use, it should have tests covering: 1) new plugin installed and ACTIVE, 2) legacy plugin installed and ACTIVE, 3) no plugin installed (returns NewSemiSyncNames), 4) error scenarios, and 5) edge cases like INACTIVE or DISABLED plugin states. This is especially important since the detection logic has a subtle bug regarding non-ACTIVE plugin states.
…ce/replica
Migrate semi-synchronous replication from the deprecated master/slave plugins to the new source/replica plugins for MySQL 9.x compatibility.
Key changes:
Existing clusters with legacy plugins continue to work because the controller detects the installed plugin at runtime. Migration happens when moco-agent re-initializes plugins on Pod restart.
Ref: #847 cybozu-go/moco-agent#117