New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Aliyun RDS #541
Support Aliyun RDS #541
Changes from 4 commits
6f3d54a
6ee5df1
bf01b31
b3f599a
d3b98d9
619f982
3dc4930
831566c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,16 +65,25 @@ func StringContainsAll(s string, substrings ...string) bool { | |
} | ||
|
||
func ValidateConnection(db *gosql.DB, connectionConfig *mysql.ConnectionConfig) (string, error) { | ||
query := `select @@global.port, @@global.version` | ||
versionQuery := `select @@global.version` | ||
var port, extraPort int | ||
var version string | ||
if err := db.QueryRow(query).Scan(&port, &version); err != nil { | ||
if err := db.QueryRow(versionQuery).Scan(&version); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The version is used in the function Inspector.validateConnection like before, this patch just prevent to get port from database. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you, you are right. |
||
return "", err | ||
} | ||
extraPortQuery := `select @@global.extra_port` | ||
if err := db.QueryRow(extraPortQuery).Scan(&extraPort); err != nil { | ||
// swallow this error. not all servers support extra_port | ||
} | ||
// AliyunRDS set users port to "NULL", replace it by gh-ost param | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this can be changed to a more general option like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point, --aliyun-rds is prevent getting port and hostname now, maybe --assume-check or something, @shlomi-noach we may need a product manager to deicide this. : ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to keep --aliyun-rds flags because we can do more things in the future. For example, the slave node is invisible for users and gh-ost cann't request binlog from one of them, maybe we can open a path for gh-ost. --assume-port also make sense, but in a different PR I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense to me to keep |
||
if Context.AliyunRDS { | ||
port = connectionConfig.Key.Port | ||
} else { | ||
portQuery := `select @@global.port` | ||
if err := db.QueryRow(portQuery).Scan(&port); err != nil { | ||
return "", err | ||
} | ||
} | ||
|
||
if connectionConfig.Key.Port == port || (extraPort > 0 && connectionConfig.Key.Port == extraPort) { | ||
log.Infof("connection validated on %+v", connectionConfig.Key) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,8 @@ func acceptSignals(migrationContext *base.MigrationContext) { | |
// main is the application's entry point. It will either spawn a CLI or HTTP interfaces. | ||
func main() { | ||
migrationContext := base.NewMigrationContext() | ||
base.Context = migrationContext | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually don't see where it is used. |
||
|
||
flag.StringVar(&migrationContext.InspectorConnectionConfig.Key.Hostname, "host", "127.0.0.1", "MySQL hostname (preferably a replica, not the master)") | ||
flag.StringVar(&migrationContext.AssumeMasterHostname, "assume-master-host", "", "(optional) explicitly tell gh-ost the identity of the master. Format: some.host.com[:port] This is useful in master-master setups where you wish to pick an explicit master, or in a tungsten-replicator where gh-ost is unable to determine the master") | ||
flag.IntVar(&migrationContext.InspectorConnectionConfig.Key.Port, "port", 3306, "MySQL port (preferably a replica, not the master)") | ||
|
@@ -67,6 +69,7 @@ func main() { | |
flag.BoolVar(&migrationContext.IsTungsten, "tungsten", false, "explicitly let gh-ost know that you are running on a tungsten-replication based topology (you are likely to also provide --assume-master-host)") | ||
flag.BoolVar(&migrationContext.DiscardForeignKeys, "discard-foreign-keys", false, "DANGER! This flag will migrate a table that has foreign keys and will NOT create foreign keys on the ghost table, thus your altered table will have NO foreign keys. This is useful for intentional dropping of foreign keys") | ||
flag.BoolVar(&migrationContext.SkipForeignKeyChecks, "skip-foreign-key-checks", false, "set to 'true' when you know for certain there are no foreign keys on your table, and wish to skip the time it takes for gh-ost to verify that") | ||
flag.BoolVar(&migrationContext.AliyunRDS, "aliyun-rds", false, "set to 'true' when you execute on Aliyun RDS.") | ||
|
||
executeFlag := flag.Bool("execute", false, "actually execute the alter & migrate the table. Default is noop: do some tests and exit") | ||
flag.BoolVar(&migrationContext.TestOnReplica, "test-on-replica", false, "Have the migration run on a replica, not on the master. At the end of migration replication is stopped, and tables are swapped and immediately swap-revert. Replication remains stopped and you can compare the two tables for building trust") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,10 +89,12 @@ func (this *Applier) InitDBConnections() (err error) { | |
if err := this.validateAndReadTimeZone(); err != nil { | ||
return err | ||
} | ||
if impliedKey, err := mysql.GetInstanceKey(this.db); err != nil { | ||
if impliedKey, err := mysql.GetInstanceKey(this.db, this.migrationContext.AliyunRDS); err != nil { | ||
return err | ||
} else { | ||
this.connectionConfig.ImpliedKey = impliedKey | ||
if this.migrationContext.AliyunRDS != true { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or also see my suggestion to replace the entire block. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your suggestion ! replace the entire block makes the code cleaner. |
||
this.connectionConfig.ImpliedKey = impliedKey | ||
} | ||
} | ||
if err := this.readTableColumns(); err != nil { | ||
return err | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,10 +53,12 @@ func (this *Inspector) InitDBConnections() (err error) { | |
if err := this.validateConnection(); err != nil { | ||
return err | ||
} | ||
if impliedKey, err := mysql.GetInstanceKey(this.db); err != nil { | ||
if impliedKey, err := mysql.GetInstanceKey(this.db, this.migrationContext.AliyunRDS); err != nil { | ||
return err | ||
} else { | ||
this.connectionConfig.ImpliedKey = impliedKey | ||
if this.migrationContext.AliyunRDS != true { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or also see my suggestion to replace the entire block. |
||
this.connectionConfig.ImpliedKey = impliedKey | ||
} | ||
} | ||
if err := this.validateGrants(); err != nil { | ||
return err | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -171,9 +171,11 @@ func GetSelfBinlogCoordinates(db *gosql.DB) (selfBinlogCoordinates *BinlogCoordi | |
} | ||
|
||
// GetInstanceKey reads hostname and port on given DB | ||
func GetInstanceKey(db *gosql.DB) (instanceKey *InstanceKey, err error) { | ||
func GetInstanceKey(db *gosql.DB, isAliyunRDS bool) (instanceKey *InstanceKey, err error) { | ||
instanceKey = &InstanceKey{} | ||
err = db.QueryRow(`select @@global.hostname, @@global.port`).Scan(&instanceKey.Hostname, &instanceKey.Port) | ||
if isAliyunRDS != true { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or also see my suggestion to replace the entire block. |
||
err = db.QueryRow(`select @@global.hostname, @@global.port`).Scan(&instanceKey.Hostname, &instanceKey.Port) | ||
} | ||
return instanceKey, err | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to get rid of this global variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The global variable is used in base/utils.go ValidateConnection. I need to get the aliyunRDS flag to avoid get the port from database. If don't use the global variable, add a param ( migrationContext may be ) to the function? or any other suggestions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it is used there. Sorry, I've missed this.
I still wish to get rid of the global variable. It will not play well with testing and embedding of
gh-ost
as a library. Please add amigrationContext *MigrationContext
argument to base/utils.goValidateConnection()
function and use it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Branch is updated, thanks for the review.