keep BackupHandle out of Params structs, document struct fiels, other minor edits

Signed-off-by: deepthi <deepthi@planetscale.com>

go/vt/mysqlctl/backup.go

@@ -100,8 +100,6 @@ func Backup(ctx context.Context, dir, name string, params BackupParams) error {
 	if err != nil {
 		return vterrors.Wrap(err, "StartBackup failed")
 	}
-	// Set params.BackupHandle to the selected backup
-	params.BackupHandle = bh
 
 	be, err := GetBackupEngine()
 	if err != nil {
@@ -109,7 +107,7 @@ func Backup(ctx context.Context, dir, name string, params BackupParams) error {
 	}
 
 	// Take the backup, and either AbortBackup or EndBackup.
-	usable, err := be.ExecuteBackup(ctx, params)
+	usable, err := be.ExecuteBackup(ctx, params, bh)
 	logger := params.Logger
 	var finishErr error
 	if usable {
@@ -291,15 +289,13 @@ func Restore(ctx context.Context, params RestoreParams) (mysql.Position, error)
 	if err != nil {
 		return rval, err
 	}
-	// Set params.BackupHandle to the selected backup
-	params.BackupHandle = bh
 
 	re, err := GetRestoreEngine(ctx, bh)
 	if err != nil {
 		return mysql.Position{}, vterrors.Wrap(err, "Failed to find restore engine")
 	}
 
-	if rval, err = re.ExecuteRestore(ctx, params); err != nil {
+	if rval, err = re.ExecuteRestore(ctx, params, bh); err != nil {
 		return rval, err
 	}
 

go/vt/mysqlctl/backupengine.go

@@ -40,40 +40,51 @@ var (
 
 // BackupEngine is the interface to take a backup with a given engine.
 type BackupEngine interface {
-	ExecuteBackup(ctx context.Context, params BackupParams) (bool, error)
+	ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, error)
 	ShouldDrainForBackup() bool
 }
 
 // BackupParams is the struct that holds all params passed to ExecuteBackup
 type BackupParams struct {
-	Cnf          *Mycnf
-	Mysqld       MysqlDaemon
-	Logger       logutil.Logger
-	BackupHandle backupstorage.BackupHandle
-	Concurrency  int
+	Cnf    *Mycnf
+	Mysqld MysqlDaemon
+	Logger logutil.Logger
+	// Concurrency is the value of -concurrency flag given to Backup command
+	// It determines how many files are processed in parallel
+	Concurrency int
+	// Extra env variables for pre-backup and post-backup transform hooks
 	HookExtraEnv map[string]string
-	TopoServer   *topo.Server
-	Keyspace     string
-	Shard        string
+	// TopoServer, Keyspace and Shard are used to discover master tablet
+	TopoServer *topo.Server
+	Keyspace   string
+	Shard      string
 }
 
 // RestoreParams is the struct that holds all params passed to ExecuteRestore
 type RestoreParams struct {
-	Cnf                 *Mycnf
-	Mysqld              MysqlDaemon
-	Logger              logutil.Logger
-	BackupHandle        backupstorage.BackupHandle
-	Concurrency         int
-	HookExtraEnv        map[string]string
-	LocalMetadata       map[string]string
+	Cnf    *Mycnf
+	Mysqld MysqlDaemon
+	Logger logutil.Logger
+	// Concurrency is the value of -restore_concurrency flag (init restore parameter)
+	// It determines how many files are processed in parallel
+	Concurrency int
+	// Extra env variables for pre-restore and post-restore transform hooks
+	HookExtraEnv map[string]string
+	// Metadata to write into database after restore. See PopulateMetadataTables
+	LocalMetadata map[string]string
+	// DeleteBeforeRestore tells us whether existing data should be deleted before
+	// restoring. This is always set to false when starting a tablet with -restore_from_backup,
+	// but is set to true when executing a RestoreFromBackup command on an already running vttablet
 	DeleteBeforeRestore bool
-	DbName              string
-	Dir                 string
+	// Name of the managed database / schema
+	DbName string
+	// Directory location to search for a usable backup
+	Dir string
 }
 
 // RestoreEngine is the interface to restore a backup with a given engine.
 type RestoreEngine interface {
-	ExecuteRestore(ctx context.Context, params RestoreParams) (mysql.Position, error)
+	ExecuteRestore(ctx context.Context, params RestoreParams, bh backupstorage.BackupHandle) (mysql.Position, error)
 }
 
 // BackupRestoreEngine is a combination of BackupEngine and RestoreEngine.

go/vt/mysqlctl/builtinbackupengine.go

@@ -239,23 +239,18 @@ func findFilesToBackup(cnf *Mycnf) ([]FileEntry, error) {
 
 // ExecuteBackup returns a boolean that indicates if the backup is usable,
 // and an overall error.
-func (be *BuiltinBackupEngine) ExecuteBackup(ctx context.Context, params BackupParams) (bool, error) {
+func (be *BuiltinBackupEngine) ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, error) {
 
 	// extract all params from BackupParams
 	cnf := params.Cnf
 	mysqld := params.Mysqld
 	logger := params.Logger
-	bh := params.BackupHandle
 	backupConcurrency := params.Concurrency
 	hookExtraEnv := params.HookExtraEnv
 	topoServer := params.TopoServer
 	keyspace := params.Keyspace
 	shard := params.Shard
 
-	if bh == nil {
-		return false, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "ExecuteBackup must be called with a valid BackupHandle")
-	}
-
 	logger.Infof("Hook: %v, Compress: %v", *backupStorageHook, *backupStorageCompress)
 
 	// Save initial state so we can restore.
@@ -353,11 +348,12 @@ func (be *BuiltinBackupEngine) ExecuteBackup(ctx context.Context, params BackupP
 			return usable, vterrors.Wrap(err, "slave is not restarting")
 		}
 
-		// wait for reliable seconds behind master
-		// we have replicationPosition where we stopped
-		// if MasterPosition is the same, that means no writes
-		// have happened to master, so we are up-to-date
-		// otherwise, wait for replica's Position to change from
+		// Wait for a reliable value for SecondsBehindMaster from SlaveStatus()
+
+		// We know that we stopped at replicationPosition.
+		// If MasterPosition is the same, that means no writes
+		// have happened to master, so we are up-to-date.
+		// Otherwise, we wait for replica's Position to change from
 		// the saved replicationPosition before proceeding
 		tmc := tmclient.NewTabletManagerClient()
 		defer tmc.Close()
@@ -365,7 +361,7 @@ func (be *BuiltinBackupEngine) ExecuteBackup(ctx context.Context, params BackupP
 		defer remoteCancel()
 
 		masterPos, err := getMasterPosition(remoteCtx, tmc, topoServer, keyspace, shard)
-		// if we are unable to get master position, return error
+		// If we are unable to get master position, return error.
 		if err != nil {
 			return usable, err
 		}
@@ -560,22 +556,15 @@ func (be *BuiltinBackupEngine) backupFile(ctx context.Context, cnf *Mycnf, mysql
 // ExecuteRestore restores from a backup. If the restore is successful
 // we return the position from which replication should start
 // otherwise an error is returned
-func (be *BuiltinBackupEngine) ExecuteRestore(
-	ctx context.Context,
-	params RestoreParams) (mysql.Position, error) {
+func (be *BuiltinBackupEngine) ExecuteRestore(ctx context.Context, params RestoreParams, bh backupstorage.BackupHandle) (mysql.Position, error) {
 
 	cnf := params.Cnf
 	mysqld := params.Mysqld
 	logger := params.Logger
-	bh := params.BackupHandle
 	restoreConcurrency := params.Concurrency
 	hookExtraEnv := params.HookExtraEnv
 
 	zeroPosition := mysql.Position{}
-	if bh == nil {
-		return zeroPosition, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "ExecuteRestore must be called with a valid BackupHandle")
-	}
-
 	var bm builtinBackupManifest
 
 	if err := getBackupManifestInto(ctx, bh, &bm); err != nil {

go/vt/mysqlctl/xtrabackupengine.go

@@ -121,16 +121,11 @@ func closeFile(wc io.WriteCloser, fileName string, logger logutil.Logger, finalE
 
 // ExecuteBackup returns a boolean that indicates if the backup is usable,
 // and an overall error.
-func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupParams) (complete bool, finalErr error) {
+func (be *XtrabackupEngine) ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (complete bool, finalErr error) {
 	// extract all params from BackupParams
 	cnf := params.Cnf
 	mysqld := params.Mysqld
 	logger := params.Logger
-	bh := params.BackupHandle
-
-	if bh == nil {
-		return false, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "ExecuteBackup must be called with a valid BackupHandle")
-	}
 
 	if *xtrabackupUser == "" {
 		return false, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "xtrabackupUser must be specified.")
@@ -372,20 +367,13 @@ func (be *XtrabackupEngine) backupFiles(ctx context.Context, cnf *Mycnf, logger
 }
 
 // ExecuteRestore restores from a backup. Any error is returned.
-func (be *XtrabackupEngine) ExecuteRestore(
-	ctx context.Context,
-	params RestoreParams) (mysql.Position, error) {
+func (be *XtrabackupEngine) ExecuteRestore(ctx context.Context, params RestoreParams, bh backupstorage.BackupHandle) (mysql.Position, error) {
 
 	cnf := params.Cnf
 	mysqld := params.Mysqld
 	logger := params.Logger
-	bh := params.BackupHandle
 
 	zeroPosition := mysql.Position{}
-	if bh == nil {
-		return zeroPosition, vterrors.New(vtrpc.Code_INVALID_ARGUMENT, "ExecuteRestore must be called with a valid BackupHandle")
-	}
-
 	var bm xtraBackupManifest
 
 	if err := getBackupManifestInto(ctx, bh, &bm); err != nil {