-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 all 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 |
---|---|---|
|
@@ -64,17 +64,26 @@ func StringContainsAll(s string, substrings ...string) bool { | |
return nonEmptyStringsFound | ||
} | ||
|
||
func ValidateConnection(db *gosql.DB, connectionConfig *mysql.ConnectionConfig) (string, error) { | ||
query := `select @@global.port, @@global.version` | ||
func ValidateConnection(db *gosql.DB, connectionConfig *mysql.ConnectionConfig, migrationContext *MigrationContext) (string, error) { | ||
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 { | ||
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 migrationContext.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) | ||
|
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 don't see where
version
is used?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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, you are right.