search

twindb / backup

TwinDB Backup
https://twindb-backup.readthedocs.io/en/stable/
Other
79 stars 44 forks source link

[Question] Hard-Coding --host=127.0.01 in mysql_source.py #184

Open ernstae opened 5 years ago

ernstae commented 5 years ago

Description

I rely on a defaults file when using twindb-backup, but on hosts with DNS lookups disabled, I would prefer to run my xtrabackup processes against the local mysql socket, instead of via TCP. Because of the hard-coded line 177 in mysql_source.py, despite what I have placed in my defaults-file, the xtrabackup process gets executed by twindb-backup with the host hard-coded to the local loopback IP.

https://github.com/twindb/backup/blob/2d3a2e94bb569974d918a79dc33c77217b735040/twindb_backup/source/mysql_source.py#L177

My question, is whether we can pull this line out of the source code? Xtrabackup, without any --host flag assumes localhost and in my case, that's the desired state for my environment, and uses the MySQL socket.

This is mostly to solicit thoughts on whether it makes sense to add a configuration file option that allows this value to be changed, instead of hard-coding the value here.

In my case, I've just removed line 177 to get the desired outcome, but am soliciting feedback before offering up a PR.

akuzminsky commented 5 years ago

I don't see why that line can't be removed. Indeed, it's better to give users more control how to connect to MySQL instance. However some users may have created backup account xxx@127.0.0.1 which will be denied if xtrabackup connects to localhost.

Potentially, we could bump the major version and declare this a backward incompatible change, but I think it's better to retry and connect to 127.0.0.1 if connecting to localhost gives "access denied".

Please send a PR if you feel like up for implementing this a in backward compatible way. Otherwise, I'll do it in the next release.

ernstae commented 5 years ago

I'm thinking the easiest way to handle it, would be to have an implicit default of 127.0.0.1, but allow an override in the configuration file. I've been dabbling in building the Azure Blob Storage support, and am feeling a bit more comfortable with the codebase, so will likely fire off a PR in the next few days for this particular small change.