Multi-cluster support: MySQL and PostgreSQL simultaneously#75
Multi-cluster support: MySQL and PostgreSQL simultaneously#75renecannao merged 1 commit intomasterfrom
Conversation
Store provider_type per instance in the database_instance table instead of relying solely on the global config.Config.ProviderType setting. This allows a single orchestrator instance to discover, analyze, and recover both MySQL and PostgreSQL topologies at the same time. Changes: - Add provider_type column to database_instance (default 'mysql') - Add ProviderType field to Instance struct - Add detectProviderType() that checks DB, then port heuristic (5432=PG), then falls back to global config for backward compatibility - Update ReadTopologyInstance/ReadTopologyInstanceBufferable to use per-instance detection instead of global config - Update ReadInstanceClusterAttributes to check instance ProviderType - Store and read provider_type in mkInsertOdkuForInstances/readInstanceRow - Set ProviderType="postgresql" during PG discovery - Merge MySQL and PG analysis results in GetReplicationAnalysis when both provider types are present in the backend DB - Update tests for new provider_type column in INSERT/UPDATE SQL Closes #73
|
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 (6)
📝 WalkthroughWalkthroughThese changes implement per-instance provider type detection and storage, moving away from a global Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant AnalysisRouter as GetReplicationAnalysis
participant MySQLCheck as hasMySQLInstances
participant PostgreSQLCheck as hasPostgreSQLInstances
participant MySQLAnalysis as GetMySQLReplicationAnalysis
participant PostgreSQLAnalysis as GetPostgreSQLReplicationAnalysis
participant Database as Database
Client->>AnalysisRouter: GetReplicationAnalysis(clusterName)
alt PostgreSQL-only Mode
AnalysisRouter->>PostgreSQLCheck: hasPostgreSQLInstances()
PostgreSQLCheck->>Database: Query provider_type='postgresql'
PostgreSQLCheck-->>AnalysisRouter: true
AnalysisRouter->>PostgreSQLAnalysis: Analyze cluster
PostgreSQLAnalysis-->>AnalysisRouter: Results
AnalysisRouter-->>Client: PostgreSQL results
else Mixed or MySQL Mode
AnalysisRouter->>MySQLAnalysis: Analyze cluster
MySQLAnalysis->>Database: Query MySQL instances
MySQLAnalysis-->>AnalysisRouter: MySQL results
AnalysisRouter->>PostgreSQLCheck: hasPostgreSQLInstances()
PostgreSQLCheck->>Database: Query provider_type='postgresql'
PostgreSQLCheck-->>AnalysisRouter: true/false
alt PostgreSQL instances exist
AnalysisRouter->>PostgreSQLAnalysis: Analyze cluster
PostgreSQLAnalysis-->>AnalysisRouter: PostgreSQL results
AnalysisRouter->>AnalysisRouter: Merge results
end
AnalysisRouter-->>Client: Merged MySQL + PostgreSQL results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ 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 |
![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 introduces support for managing both MySQL and PostgreSQL instances by adding a "provider_type" field to the database schema and instance metadata. It implements a detection mechanism using port heuristics and database lookups, updates replication analysis to aggregate results from both providers, and includes corresponding test updates. Review feedback suggests optimizing performance by caching provider type detection results to reduce database load and removing redundant lookups in the discovery workflow where the provider type is already known.
| var instanceReadChan = make(chan bool, backendDBConcurrency) | ||
| var instanceWriteChan = make(chan bool, backendDBConcurrency) |
There was a problem hiding this comment.
To avoid frequent and redundant database queries to the orchestrator backend during the discovery process, consider introducing a cache for the instance provider type. This is especially important as detectProviderType is called multiple times per discovery cycle for each instance.
| var instanceReadChan = make(chan bool, backendDBConcurrency) | |
| var instanceWriteChan = make(chan bool, backendDBConcurrency) | |
| var instanceReadChan = make(chan bool, backendDBConcurrency) | |
| var instanceWriteChan = make(chan bool, backendDBConcurrency) | |
| var providerTypeCache = cache.New(time.Hour, time.Hour) | |
| func detectProviderType(instanceKey *InstanceKey) string { | ||
| // Check if we already know this instance's provider type from the backend DB | ||
| query := `SELECT provider_type FROM database_instance WHERE hostname = ? AND port = ?` | ||
| var providerType string | ||
| err := db.QueryOrchestrator(query, sqlutils.Args(instanceKey.Hostname, instanceKey.Port), func(m sqlutils.RowMap) error { | ||
| providerType = m.GetString("provider_type") | ||
| return nil | ||
| }) | ||
| if err == nil && providerType != "" { | ||
| return providerType | ||
| } | ||
|
|
||
| // Port-based heuristic for new instances | ||
| if instanceKey.Port == 5432 { | ||
| return "postgresql" | ||
| } | ||
|
|
||
| // Fall back to global config (backward compatibility) | ||
| return config.Config.ProviderType | ||
| } |
There was a problem hiding this comment.
The detectProviderType function performs a synchronous database query every time it's called. Implementing caching here will significantly reduce the load on the orchestrator backend database during discovery. Additionally, the logic can be simplified to consolidate the return paths.
func detectProviderType(instanceKey *InstanceKey) string {
cacheKey := instanceKey.StringCode()
if val, found := providerTypeCache.Get(cacheKey); found {
return val.(string)
}
var providerType string
// Check if we already know this instance's provider type from the backend DB
query := `SELECT provider_type FROM database_instance WHERE hostname = ? AND port = ?`
err := db.QueryOrchestrator(query, sqlutils.Args(instanceKey.Hostname, instanceKey.Port), func(m sqlutils.RowMap) error {
providerType = m.GetString("provider_type")
return nil
})
if err == nil && providerType != "" {
// Found in DB
} else if instanceKey.Port == 5432 {
providerType = "postgresql"
} else {
providerType = config.Config.ProviderType
}
providerTypeCache.Set(cacheKey, providerType, cache.DefaultExpiration)
return providerType
}| // PostgreSQL provider sets ClusterName directly during discovery. | ||
| // Skip the master-lookup logic which is MySQL-specific. | ||
| if config.Config.ProviderType == "postgresql" { | ||
| if instance.ProviderType == "postgresql" || detectProviderType(&instance.Key) == "postgresql" { |
There was a problem hiding this comment.
Calling detectProviderType here is redundant if instance.ProviderType is already populated (which it should be after discovery or loading from the DB). Even with caching, it's better to avoid the extra function call and map lookup. You can also use this opportunity to ensure instance.ProviderType is initialized if it happens to be empty.
if instance.ProviderType == "" {
instance.ProviderType = detectProviderType(&instance.Key)
}
if instance.ProviderType == "postgresql" {
1 participant
Summary
provider_typecolumn todatabase_instancetable, replacing the globalconfig.Config.ProviderTypeas the sole routing mechanism for discovery, analysis, and recoveryDetails
Schema: New
provider_type varchar(20) NOT NULL DEFAULT 'mysql'column added via migration patch.Discovery:
detectProviderType()checks the backend DB first, then uses port-based heuristic, then falls back toconfig.Config.ProviderType. This means existing MySQL-only and PG-only deployments work without config changes.Analysis:
GetReplicationAnalysis()now runs both MySQL and PostgreSQL analysis when instances of both types exist, merging results. When only one type is present, it runs the appropriate single analysis path.Recovery: Already routes per-instance via
AnalysisCode(e.g.,DeadPrimaryfor PG,DeadMasterfor MySQL) -- no changes needed.Test plan
go test ./go/... -vet=off)go build -o /dev/null ./go/cmd/orchestrator)Closes #73
Summary by CodeRabbit