nomycnf: operate without mycnf more naturally#4094
nomycnf: operate without mycnf more naturally#4094sougou merged 3 commits intovitessio:masterfrom sougou:nomycnf
Conversation
Addresses #4087 With this change, a cnf is not needed for vttablet to operate. If any connection parameter is specified: socket or host & port, then vttablet assumes that the mysql is external, and does not attempt to load the cnf file. Consequently, this disables vttablet from performing backups and restores. In such cases, vttablet crashes if restore_from_backup is set to true. If no connection parameters are specifed, vttablet behaves like before: look for a mycnf file, and fail if one cannot be found. Of course, the cnf file fields can also be explicitly passed in explicitly. In this case, the cnf's socket file is used to connect to MySQL. Implementation: mycnf has been removed from mysqld. It's now passed as input parameter for operations that need it. The purpose of this change is to clearly document which operations of mysqld require a mycnf. The ActionAgent stores Cnf as a separate field, and checks for null value before invoking mysqld functions that require it. At this point, only Backup and Restore require it. There's a quirky corner case where a StopSlave command saves a marker file in the tree. That part has been changed to save that file only if a Cnf is present. The more correct behavior would be to store that in the topo, which is beyond the scope of this change. tabletDir was derived from mycnf and stored as a field in mysqld. The wrapper function TabletDir has now been moved into Mycnf and call sites have been changed accordingly. Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
jvaidya
left a comment
jvaidya
left a comment
There was a problem hiding this comment.
Looks good on the whole. Could you add three tests to cover the changed behaviour when mycnf is not specified and manage-backups (?) has been specified?
go/vt/mysqlctl/cmd.go
Outdated
| // installation that already exists. This will look for an existing my.cnf file | ||
| // and use that to call NewMysqld(). | ||
| func OpenMysqld(tabletUID uint32) (*Mysqld, error) { | ||
| // and return a corresponding *Mycnf. |
| dbcfgs, err := dbconfigs.Init(mycnf.SocketFile) | ||
| // If connection parameters were not specified, we'll use | ||
| // mycnf's socket file (if present). | ||
| dbcfgs, err := dbconfigs.Init(socketFile) |
There was a problem hiding this comment.
Can you change the comment to make it explicit that socketFile is unitialiazed here in the case dbconfigs.HasConnectionParams, but this is fine because it will be used only in the case where dbconfigs.HasConnectionParams is False and that case has been dealt with above.
go/vt/mysqlctl/cmd.go
Outdated
| // installation that hasn't been set up yet. This will generate a new my.cnf | ||
| // from scratch and use that to call NewMysqld(). | ||
| func CreateMysqld(tabletUID uint32, mysqlSocket string, mysqlPort int32) (*Mysqld, error) { | ||
| // installation that hasn't been set up yet. This will additionally generate |
There was a problem hiding this comment.
"returns a Mysqld object and and a Mycnf object. The MyCnf object is ...."
There was a problem hiding this comment.
Renamed these functions to ..MysqldAndMycnf and updated the comments.
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
sougou
left a comment
sougou
left a comment
There was a problem hiding this comment.
Comments addressed.
There's currently no framework to test incorrect command line flags, because the error goes to the logs. I did test it manually to ensure that it failed as expected.
go/vt/mysqlctl/cmd.go
Outdated
| // installation that hasn't been set up yet. This will generate a new my.cnf | ||
| // from scratch and use that to call NewMysqld(). | ||
| func CreateMysqld(tabletUID uint32, mysqlSocket string, mysqlPort int32) (*Mysqld, error) { | ||
| // installation that hasn't been set up yet. This will additionally generate |
There was a problem hiding this comment.
Renamed these functions to ..MysqldAndMycnf and updated the comments.
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
jvaidya
left a comment
jvaidya
left a comment
There was a problem hiding this comment.
Thanks for addressing comments. LGTM.
2 participants
Addresses #4087
With this change, a cnf is not needed for vttablet to operate.
If any connection parameter is specified: socket or host & port,
then vttablet assumes that the mysql is external, and does not
attempt to load the cnf file.
Consequently, this disables vttablet from performing backups and
restores. In such cases, vttablet crashes if restore_from_backup
is set to true.
If no connection parameters are specifed, vttablet behaves like
before: look for a mycnf file, and fail if one cannot be found.
Of course, the cnf file fields can also be explicitly passed in
explicitly. In this case, the cnf's socket file is used to connect
to MySQL.
Implementation:
mycnf has been removed from mysqld. It's now passed as input
parameter for operations that need it. The purpose of this change
is to clearly document which operations of mysqld require
a mycnf. The ActionAgent stores Cnf as a separate field, and
checks for null value before invoking mysqld functions that
require it. At this point, only Backup and Restore require it.
There's a quirky corner case where a StopSlave command saves a
marker file in the tree. That part has been changed to save
that file only if a Cnf is present. The more correct behavior
would be to store that in the topo, which is beyond the scope
of this change.
tabletDir was derived from mycnf and stored as a field in
mysqld. The wrapper function TabletDir has now been moved
into Mycnf and call sites have been changed accordingly.
Signed-off-by: Sugu Sougoumarane ssougou@gmail.com