-
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
Conversation
ilegal values. Add --aliyun-rds flag to avoid getting them.
Great,hope this PR could be merged into the trunk ASAP!:) |
I'm very sorry for the delay. I'll be looking into this in more depth shortly. |
Sorry for the delay, I'll be reviewing this shortly. |
@shlomi-noach Any progress ? |
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 for the PR!
Few notes:
- unused variable
version
- unused global variable
Context
The entire block:
if impliedKey, err := mysql.GetInstanceKey(this.db, this.migrationContext.AliyunRDS); err != nil {
return err
} else {
if this.migrationContext.AliyunRDS != true {
this.connectionConfig.ImpliedKey = impliedKey
}
}
confuses me greatly. Can we not simplify it into:
if !this.migrationContext.AliyunRDS {
if impliedKey, err := mysql.GetInstanceKey(this.db); err != nil {
return err
} else {
this.connectionConfig.ImpliedKey = impliedKey
}
}
and so also get rid of the change to GetInstanceKey()
?
go/mysql/utils.go
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Please change to if !isAliyunRDS {
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.
Or also see my suggestion to replace the entire block.
go/logic/inspect.go
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Please change to if !isAliyunRDS {
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.
Or also see my suggestion to replace the entire block.
go/base/context.go
Outdated
@@ -214,6 +215,8 @@ type ContextConfig struct { | |||
} | |||
} | |||
|
|||
var Context *MigrationContext |
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 a migrationContext *MigrationContext
argument to base/utils.go ValidateConnection()
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.
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 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.
go/cmd/gh-ost/main.go
Outdated
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the base.Context
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.
I actually don't see where it is used.
go/logic/applier.go
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Please change to if !isAliyunRDS {
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.
Or also see my suggestion to replace the entire block.
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.
Thanks for your suggestion ! replace the entire block makes the code cleaner.
support-aliyun-rds branch.
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.
Looking good. I'll send to testing.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this can be changed to a more general option like assume port (int)
or validate port (bool)
? Then it is not necessary passing migrationContext
here.
Besides, mysql running in docker with NAT would also report different port. This kind of option would be useful for similar cases.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
--assume-port
sounds good.
Please feel free to choose whether you want to change this in this PR or in a different PR, and let me know.
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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me to keep --aliyun-rds
.
./gh-ost 2018-06-07 16:17:53 ERROR Error 1885: OPERATION need to be executed set by ADMIN. aliyun rds 读写账号 不可以使用gh-ots 吗? |
@zhangxiaojian |
Related issue: #470
Description
This PR makes gh-ost to support Aliyun RDS.
Aliyun RDS is the largest cloud database in China, and the third of the world. For the reason of security and architecture , we need to hide some mysql variables so gh-ost will get ilegal values. This PR add --aliyun-rds flag to avoid getting them.
Usage
For now, users can't connect to the slave node of rds, so gh-ost may working directly on master with flags:
Thanks for gh-ost team developed an excellent tools for online DDL, and hope more people can use it on cloud database.
script/cibuild
returns with no formatting errors, build errors or unit test errors.