refactor backup code to allow us to plugin other backup engines

[deepthi]

I have pulled the refactor out of the WIP #4614 for ease of review and better commit history.
This is ready for review.

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

[rafael]

@deepthi - This makes sense and goes in the direction of what we will need for xtrabackups. I have one follow up comment:

How would backup_engine_implementation play with existent flag backup_storage_implementation ? For now it seems like we will have to pass both. I think that could be a bit confusing from the user perspective.

I wonder if we should think of a way where all these implementation will converge under backup_engine_implementation

One idea towards this direction. We have what was introduced here: backup_engine_implementation. Builtin is not registered as an engine, but rather is just an interface.

S3 and File backup storage become implementations of the builtin engine and they are the ones that register as backup_engine_implementation. That way we could have an s3 engine and a file engine. Then we could deprecate backup_storage_implementation

@deepthi what do you think of this idea ?

[deepthi]

@rafael backp_engine_implementation and backup_storage_implementation are intended to be independent. IOW, just as today you can use the builtin backup engine but store the backup in any of {file, s3, ceph, gcs}, with xtrabackup (or any other future backup engine), you should be able to store the backup in any of the storage options.

If you look at the way the code is structured, the storage portion was already nicely abstracted so this was a natural evolution of the design.

And yes, it is an additional flag for the user, but that was anyway going to be unavoidable when we introduce xtrabackup as an option.

[rafael]

I can see that and in fact seems like a natural evolution of the design.

I think the part I'm worried is the complexity of configuring backups. It becomes a matrix where you to have configure engine + storage backend. I was thinking that having this be driven by single flag was simpler from an user perspective.

In that way, we could still have the abstraction of the storage, but not exposed to Vitess users.

I would find more intuitive to have something like:

builtin_s3
builtin_file
xtrabackups_s3

Than having to look which engine and then, which storage inside those engines are supported.