Add flag to wait for backup instead of starting up empty.

[enisoc]

This allows shard bootstrap workflows that start up tablets concurrently while uploading an initial backup. Once the initial backup is complete, the tablets will restore from it and start serving. Without this, the workflow has to wait for the backup to complete before launching any tablets, which makes it slower and more complex.

Signed-off-by: Anthony Yeh enisoc@planetscale.com

[teejae]

Suggested change

	If the `-wait_for_backup_interval` flag is set to a value greater than zero,
	If the `-init_from_backup_retry_interval` flag is set to a value greater than zero,

[enisoc]

The name "retry" is not accurate here. The current behavior of -restore_from_backup is that "no backup exists; starting up empty" is considered a normal, non-error result. It doesn't make sense that adding a retry interval will change this semantic because retries only apply to errors.

[teejae]

but you're retrying until success. that's generally what retries are for. this is changing the behavior of restore_from_backup, which only is a single time special case, and happens to startup from empty.

[enisoc]

but you're retrying until success

The behavior I'm adding is not "retrying until success". If the backup doesn't exist, "success" is currently defined as "start up empty". There is no need nor expectation to retry anything. The thing you tried (-restore_from_backup, which implicitly means "restore from backup if there is one or else start up empty") succeeded.

I need a name that indicates I'm redefining what success means, not just adding a retry until the existing definition of success is met.

this is changing the behavior of restore_from_backup, which only is a single time special case, and happens to startup from empty.

I don't follow what this means. Can you reword it if I've missed the point?

[teejae]

i must misunderstand the code, but it seems to look for backups until it finds one. what else does init_from_backup_retry_interval mean? what other success is there? if it can't find a backup, it keeps retrying right? it doesn't start up empty anymore.

[enisoc]

If you look at the new behavior in isolation, the word "retry" makes sense. However, if you take into account the context of how the previous behavior worked -- what users have had to understand since -restore_from_backup was first introduced -- then the word "retry" is inaccurate and misleading. In other words, we can't call this "retry" because of legacy.

I'm open to other names that are better than wait_for_backup, but not if the name actively misleads users.

[teejae]

Suggested change

	waitForBackupInterval = flag.Duration("wait_for_backup_interval", 0, "(init restore parameter) if this is greater than 0, instead of starting up empty when no backups are found, keep checking at this interval for a backup to appear")
	restoreFromBackupRetryInterval = flag.Duration("init_from_backup_retry_interval", 0, "(init restore parameter) when greater than 0, continuously retry restoring backup until success before starting up")

if we want to keep with the bad name restore_from_backup, we can also name this restore_from_backup_retry_interval.

[teejae]

Suggested change

	log.Infof("No backup found. Waiting %v (from -wait_for_backup_interval flag) to check again.", waitForBackupInterval)
	log.Infof("No backup found. Retrying in %v (from -wait_for_backup_interval flag).", waitForBackupInterval)

[teejae]

Suggested change

	err = agent.restoreDataLocked(ctx, l, 0 /* waitForBackupInterval */, true /* deleteBeforeRestore */)
	retryInterval := 0*time.Seconds
	err = agent.restoreDataLocked(ctx, l, 0 retryInterval, true /* deleteBeforeRestore */)

[sougou]

Note that user documentation under /doc is deprecated. They should all go to vitessio/website. I intend to delete most files in that directory except for design docs that may be relevant to contributors.

[enisoc]

Ah thanks for the heads-up @sougou. I'll open a PR against vitessio/website after this is approved.

[deepthi]

It seems that with the flag set, we will keep looking for a backup at intervals until the context times out or we find a good backup (whichever comes first). Has there been any thought or discussion as to whether the timeout should be shorter than what is passed in?

[enisoc]

@deepthi Do you mean, voluntarily set a shorter timeout for the "wait for backup" stage so we have a chance to still do something after that times out, before the overall context times out?

Normally that's a good practice, but I don't think it applies in this case. The intended meaning of "wait for backup" is "never proceed to start up until you successfully restore from a backup"; the idea is to provide a guarantee that the tablet will never start up empty. So if we time out while waiting for the backup, we're not allowed to do anything else besides quit anyway.