Make port and host optional for mysql plugin

[jovandeginste]

Pull Request (PR) description

When we use a socket, the host and port should be empty (or omitted).

This Pull Request (PR) fixes the following issues

n/a

Signed-off-by: Jo Vandeginste [email protected]

[jovandeginste]

This is not a backward-compatible change: empty host/port resolved to the default value (localhost/3306) before, while now they are discarded.

The alternative, is to allow the empty string as host. That is how I used to fix this issue in my setup, before upgrading to this version of the module... (However, now the type is too strict, and complains about empty string as host)

[ghoneycutt]

I would not classify this as backwards-incompatible because the type change still honors the old value and just extends it to allow undef.

[jovandeginste]

Thanks for spotting this, @ghoneycutt; I was too hasty with the tests 😄

[bastelfreak]

@ghoneycutt why do you think it's not backwards-incompatible? The defaults localhost/3306 got removed. Or does the collectd use that by default because it's compiled into the plugin?

[jovandeginste]

@ghoneycutt why do you think it's not backwards-incompatible? The defaults localhost/3306 got removed. Or does the collectd use that by default because it's compiled into the plugin?

I only checked now, but it seems it will: the manpage

Host Hostname
Hostname of the database server. Defaults to localhost.

Port Port
TCP-port to connect to. The port must be specified in its numeric form, but it must be passed as a string nonetheless. For example:
Port "3306"
If Host is set to localhost (the default), this setting has no effect. See the documentation for the mysql_real_connect function for details.

(and for further reference)

Socket Socket
Specifies the path to the UNIX domain socket of the MySQL server. This option only has any effect, if Host is set to localhost (the default). Otherwise, use the Port option above. See the documentation for the mysql_real_connect function for details.

[jovandeginste]

Though I'm not sure about the port part, @bastelfreak

[ghoneycutt]

The change in the data type is OK though the default value should not change. This ensures it is backwards compatible while still allowing you to specify things differently for your use case.

[jovandeginste]

I don't think I understand; how can you allow undef and have a default value?

[smortex]

A value with a default cannot be set to undef.

The underlaying question is: If I do not set a port, and if I set port to 3306, is the result the same? For what I understand, the default Port 3306 is now removed from the configuration, but if no port is the same as port 3306, it is not a breaking change because the bahaviour is the same.

If the behavior change (e.g. metrics are not collected, the name of the metric change, etc), then this is backward incompatible.

[jovandeginste]

For the port, you are correct; we could add back the default value there. For the host, however, the behavior is different...

[jovandeginste]

@ghoneycutt since this PR has been merged, what exactly would you like me to do now?

[smortex]

Well, I would recommend to do nothing: the issue is marked as backward incompatible so the next version will be a major one which is appropriate 😃.