Support Aliyun RDS by zhangxiaojian · Pull Request #541 · github/gh-ost

zhangxiaojian · 2018-01-30T03:07:47Z

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:

--allow-on-master
--assume-rbr
--assume-master-host
--aliyun-rds

Thanks for gh-ost team developed an excellent tools for online DDL, and hope more people can use it on cloud database.

contributed code is using same conventions as original code
script/cibuild returns with no formatting errors, build errors or unit test errors.

ilegal values. Add --aliyun-rds flag to avoid getting them.

dikang123 · 2018-02-01T14:12:31Z

Great,hope this PR could be merged into the trunk ASAP!:)

shlomi-noach · 2018-02-19T12:17:12Z

I'm very sorry for the delay. I'll be looking into this in more depth shortly.

shlomi-noach · 2018-03-07T14:38:49Z

Sorry for the delay, I'll be reviewing this shortly.

zhangxiaojian · 2018-03-27T11:16:02Z

@shlomi-noach Any progress ?

shlomi-noach

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()?

shlomi-noach · 2018-03-27T11:20:08Z

go/mysql/utils.go

+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 {


Please change to if !isAliyunRDS {

Or also see my suggestion to replace the entire block.

shlomi-noach · 2018-03-27T11:21:04Z

go/logic/inspect.go

 		return err
 	} else {
-		this.connectionConfig.ImpliedKey = impliedKey
+		if this.migrationContext.AliyunRDS != true {


Please change to if !isAliyunRDS {

Or also see my suggestion to replace the entire block.

shlomi-noach · 2018-03-27T11:26:46Z

go/base/context.go

 	}
 }

+var Context *MigrationContext


I'd like to get rid of this global variable.

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 ?

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.

Branch is updated, thanks for the review.

shlomi-noach · 2018-03-27T11:27:37Z

go/base/utils.go

 	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 {


I don't see where version is used?

The version is used in the function Inspector.validateConnection like before, this patch just prevent to get port from database.

Thank you, you are right.

shlomi-noach · 2018-03-27T11:28:13Z

go/cmd/gh-ost/main.go

 // main is the application's entry point. It will either spawn a CLI or HTTP interfaces.
 func main() {
 	migrationContext := base.NewMigrationContext()
+	base.Context = migrationContext


please remove the base.Context global variable.

I actually don't see where it is used.

shlomi-noach · 2018-03-27T11:30:42Z

go/logic/applier.go

 		return err
 	} else {
-		this.connectionConfig.ImpliedKey = impliedKey
+		if this.migrationContext.AliyunRDS != true {


Please change to if !isAliyunRDS {

Or also see my suggestion to replace the entire block.

Thanks for your suggestion ! replace the entire block makes the code cleaner.

support-aliyun-rds branch.

shlomi-noach

Looking good. I'll send to testing.

everpcpc · 2018-03-29T05:05:37Z

go/base/utils.go

 	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


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.

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. : )

--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.

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.

Makes sense to me to keep --aliyun-rds.

gaotuan · 2018-06-07T08:25:08Z

./gh-ost
--aliyun-rds
--ok-to-drop-table
--initially-drop-ghost-table
--initially-drop-socket-file
--host="xx.mysql.singapore.rds.aliyuncs.com"
--port=3306
--user="xx"
--password="xx"
--database="xx"
--table="xxx"
--verbose
--alter="MODIFY COLUMN device_info text,add COLUMN country_code varchar(32) COMMENT '国家编码';"
--panic-flag-file=/tmp/ghost.panic.flag
--allow-on-master
--throttle-flag-file /tmp/1.log
–-assume-rbr=true –-assume-master-host=xx.xx:3306 --execute

2018-06-07 16:17:53 ERROR Error 1885: OPERATION need to be executed set by ADMIN.
2018-06-07 16:17:53 ERROR Error 1885: OPERATION need to be executed set by ADMIN.
2018-06-07 16:17:53 INFO Tearing down inspector
2018-06-07 16:17:53 FATAL Error 1885: OPERATION need to be executed set by ADMIN.

aliyun rds 读写账号不可以使用gh-ots 吗？

wps353 · 2018-06-21T07:56:44Z

@zhangxiaojian
If aliyun RDS database name including special character,such as '_'.
this tool still not working.

Aliyun RDS hide some mysql variables for secrity so gh-ost will get

6f3d54a

ilegal values. Add --aliyun-rds flag to avoid getting them.

Merge branch 'master' into support-aliyun-rds

6ee5df1

Merge branch 'master' into support-aliyun-rds

bf01b31

Merge branch 'master' into support-aliyun-rds

b3f599a

shlomi-noach suggested changes Mar 27, 2018

View reviewed changes

Refactor the code and remove the global variable of

d3b98d9

support-aliyun-rds branch.

shlomi-noach reviewed Mar 29, 2018

View reviewed changes

everpcpc reviewed Mar 29, 2018

View reviewed changes

shlomi-noach approved these changes Apr 8, 2018

View reviewed changes

Shlomi Noach added 3 commits April 12, 2018 09:33

Merge branch 'master' into support-aliyun-rds

619f982

Merge branch 'master' into support-aliyun-rds

3dc4930

Merge branch 'master' into support-aliyun-rds

831566c

shlomi-noach merged commit f914b93 into github:master Apr 16, 2018

shlomi-noach mentioned this pull request May 14, 2018

enhancement:does it support aliyun rds? #470

Closed

zhangxiaojian deleted the support-aliyun-rds branch June 5, 2018 02:57

Conversation

zhangxiaojian commented Jan 30, 2018

Description

Usage

Uh oh!

dikang123 commented Feb 1, 2018

Uh oh!

shlomi-noach commented Feb 19, 2018

Uh oh!

shlomi-noach commented Mar 7, 2018

Uh oh!

zhangxiaojian commented Mar 27, 2018

Uh oh!

shlomi-noach left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shlomi-noach left a comment

Choose a reason for hiding this comment

Uh oh!

everpcpc Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaotuan commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wps353 commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shlomi-noach left a comment •

edited

Loading

everpcpc Mar 29, 2018 •

edited

Loading

gaotuan commented Jun 7, 2018 •

edited

Loading

wps353 commented Jun 21, 2018 •

edited

Loading