Skip to content

This fixes issue #34. Using the wrong hostname. #36

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

Merged
merged 3 commits into from
Jan 26, 2016

Conversation

gdey
Copy link
Contributor

@gdey gdey commented Jan 22, 2016

When registering as a slave we are using the wrong hostname. It should
be our hostname, not the hostname name of the server we are connecting
to.

Fixed issue with using the wrong variable.

When registering as a slave we are using the wrong hostname. It should
be our hostname, not the hostname name of the server we are connecting
to.

Fixed issue with using the wrong variable.
user string
password string
// LocalHost is the name of you want to present to the MySQL master. If it is not set it will default to os.Hostname()
LocalHost string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should name it localHost or local?
lower case first.

btw, where to set this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can change that. I was going to leave this as public, so that people can set this if they wanted. If they don't set it, it would default to os.Hostname(). If they set it, that's what we would send.

Here is where the check is done.
https://github.com/gdey/go-mysql/blob/2818d7e7c262a2307607238300e79c9033a3d027/replication/binlogsyncer.go#L68-L75

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think supplying SetLocalHostname() function to do this, not export struct member may be better. :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gdey, how do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll make the change.

@gdey
Copy link
Contributor Author

gdey commented Jan 22, 2016

In #35 (comment) @siddontang said:

Hi @gdey , I guess if we don't register slave, we can still sync binlog from mysql master.
Maybe we can remove register slave later?

I'm not sure what you mean? I know I need register slave for what I'm doing. I do want to register as a slave; and receive the events. (Not just look at the binlog files.) However, what I discovered was that the name in the show slaves tables was wrong. It was showing the hostname of the master instead of the hostname of the slave. This is because we are sending the wrong hostname. This pull request fixes that. I not very good at naming things. And there were two ways I could have gone with this pull request. I took the more complicated route.

  1. Just grab the hostname from the os; using os.Hostname. In the writeRegisterSlave routine.
  2. Setup a variable that people can set if they want something different, and if the variable is the zero value (i.e. "") then use os.Hostname.

I went with option two just because it offers a bit more flexibility without too much added complexity. But willing to change it.

@@ -62,6 +65,15 @@ func NewBinlogSyncer(serverID uint32, flavor string) *BinlogSyncer {
return b
}

func (b *BinlogSyncer) LocalHostname() string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not export this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll make this lower cases. I'll change the name to localhostName().

@siddontang
Copy link
Collaborator

Hi @gdey

What I mean is that we can sync mysql binlog too even we don't send register slave command.
So I am not sure whether to support this or not. :-)

@gdey
Copy link
Contributor Author

gdey commented Jan 22, 2016

Well, the current implementation of register slave is wrong according to the docs. As it should be sending the slave's hostname, and not the hostname of the server it's connecting to. (or empty if we don't want to register as a slave).

For me; I'm connecting as a slave, and so it's useful to have it register properly.

from https://dev.mysql.com/doc/internals/en/com-register-slave.html:

COM_REGISTER_SLAVE:
register a slave at the master

Payload
1 [15] COM_REGISTER_SLAVE
4 server-id
1 slaves hostname length
string[$len] slaves hostname
1 slaves user len
string[$len] slaves user
1 slaves password len
string[$len] slaves password
2 slaves mysql-port
4 replication rank
4 master-id
Note that the value of [15] is hexadecimal; in decimal, this would be 21.

Fields
server_id (4) -- the slaves server-id

slave_hostname (string.var_len) -- see --report-host, usually empty

slave_user (string.var_len) -- see --report-user, usually empty

slave_password (string.var_len) -- see --report-password, usually empty

slave_port (2) -- see --report-port, usually empty

replication_rank (4) -- ignored

master_id (4) -- usually 0. Appears as "master id" in SHOW SLAVE HOSTS on the master. Unknown what else it impacts.

Returns
OK_Packet or ERR_Packet

https://dev.mysql.com/doc/refman/5.7/en/replication-options-slave.html#option_mysqld_report-host

gdey added 2 commits January 22, 2016 06:49
Made the LocalHost variable private and changed it's name to
localhost.

Added additional funciton SetLocalHostname to set the hostname to
register as.

Change getter methods name to match setter. Change it to
LocalHostname().
siddontang added a commit that referenced this pull request Jan 26, 2016
This fixes issue #34. Using the wrong hostname.
@siddontang siddontang merged commit 339da78 into go-mysql-org:master Jan 26, 2016
@siddontang
Copy link
Collaborator

LGTM

@lwfofgit
Copy link

lwfofgit commented Dec 3, 2020

请问一下,LocalHost可以不设置吗?默认的也不设置

@gdey
Copy link
Contributor Author

gdey commented Dec 8, 2020

@lwfofgit
您应该能够使用NewBinlogSyncer.SetHostName将其设置为所需的内容。 如果未设置,它将返回“ os.Hostname”返回的内容。

我用google translation来翻译,所以不清楚。

@AleksandrShibanov
Copy link

@lwfofgit 您应该能够使用NewBinlogSyncer.SetHostName将其设置为所需的内容。 如果未设置,它将返回“ os.Hostname”返回的内容。
我用google translation来翻译,所以不清楚。

BTW there is no function SetHostName. So how can I set LocalHostname?

@lance6716
Copy link
Collaborator

lance6716 commented Jan 23, 2023

@lwfofgit 您应该能够使用NewBinlogSyncer.SetHostName将其设置为所需的内容。 如果未设置,它将返回“ os.Hostname”返回的内容。
我用google translation来翻译,所以不清楚。

BTW there is no function SetHostName. So how can I set LocalHostname?

Seems it have been moved into a public config item

Localhost string

@AleksandrShibanov
Copy link

AleksandrShibanov commented Jan 23, 2023

@lwfofgit 您应该能够使用NewBinlogSyncer.SetHostName将其设置为所需的内容。 如果未设置,它将返回“ os.Hostname”返回的内容。
我用google translation来翻译,所以不清楚。

BTW there is no function SetHostName. So how can I set LocalHostname?

Seems it have been moved into a public config item

Localhost string

I use this config implicitly when creating syncer in Canal, here: https://github.com/go-mysql-org/go-mysql/blob/master/canal/canal.go#L435
Could you please add this option to canal config and init it here https://github.com/go-mysql-org/go-mysql/blob/master/canal/canal.go#L452?

Smth like:
Localhost: c.cfg.Localhost ?

@AleksandrShibanov
Copy link

Hi @lance6716 , i opened related pull request #777

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants