Conversation
…ctually performing a restore Signed-off-by: deepthi <deepthi@planetscale.com>
deepthi
changed the title
sougou
left a comment
sougou
left a comment
There was a problem hiding this comment.
One question. Everything else looks good.
| backupManifest, err = mysqlctl.Restore(ctx, params) | ||
| if waitForBackupInterval == 0 { | ||
| break | ||
| if mysqlctl.ShouldRestore(ctx, params) { |
There was a problem hiding this comment.
Can you see if this will read better if the check was moved to handleRestore in tm_init.go?
There was a problem hiding this comment.
Better to keep it here so that calls from other call-sites also benefit from this change.
Signed-off-by: deepthi <deepthi@planetscale.com>
|
|
||
| func (tm *TabletManager) restoreDataLocked(ctx context.Context, logger logutil.Logger, waitForBackupInterval time.Duration, deleteBeforeRestore bool) error { | ||
|
|
||
| tablet := tm.Tablet() |
There was a problem hiding this comment.
@sougou said yesterday that we shouldn't be using tm.Tablet() (display state) for programmatic decision making. Did we change our minds about that principle, or is this more of a temporary workaround until we can do a more thorough audit of all uses?
There was a problem hiding this comment.
We found that it is being used in too many places, so it is better to make sure that is returns the most up-to-date state.
| // ShouldRestore checks whether a database with tables already exists | ||
| // and returns whether a restore action should be performed | ||
| func ShouldRestore(ctx context.Context, params RestoreParams) bool { | ||
| if !params.DeleteBeforeRestore { |
There was a problem hiding this comment.
Now that this is in a function, it might be easier to read like:
if params.DeleteBeforeRestore || RestoreWasInterrupted(params.Cnf) {
return true
}
// check other stuff|
|
||
| // ShouldRestore checks whether a database with tables already exists | ||
| // and returns whether a restore action should be performed | ||
| func ShouldRestore(ctx context.Context, params RestoreParams) bool { |
There was a problem hiding this comment.
I'd feel better if we returned (bool, error) so the caller can distinguish between "we should restore" and "we failed to determine whether we should restore". That's how Restore() itself currently works. Is there a reason you thought this one should be different?
There was a problem hiding this comment.
Also would it make sense for Restore() to call ShouldRestore()?
There was a problem hiding this comment.
I went back and forth between returning an error or not. Since you feel that is better, I'll change it.
| ok, _ := checkNoDB(ctx, params.Mysqld, params.DbName) | ||
| if !ok { | ||
| params.Logger.Infof("Auto-restore is enabled, but mysqld already contains data. Assuming vttablet was just restarted.") | ||
| _ = PopulateMetadataTables(params.Mysqld, params.LocalMetadata, params.DbName) |
There was a problem hiding this comment.
It seems unexpected that a function called ShouldRestore would have side effects like this. Unless ShouldRestore is called from many places, I'd suggest moving the call to PopulateMetadataTables to the call site so the either/or logic is clearer: either we restore, or we merely populate metadata.
| backupManifest, err = mysqlctl.Restore(ctx, params) | ||
| if waitForBackupInterval == 0 { | ||
| break | ||
| if mysqlctl.ShouldRestore(ctx, params) { |
There was a problem hiding this comment.
I feel like this would read better as an early return since we really want to skip the rest of the function.
if !mysqlctl.ShouldRestore(ctx, params) {
// Just populate metadata and then return.
}
// Do actual restore.
|
Closing this, I'll open two separate PRs, one for the TM changes and one for the restore changes. |
3 participants
Also, do not change type to RESTORE if not actually performing a restore.
Signed-off-by: deepthi deepthi@planetscale.com