More refactoring of backup code. #3515
Conversation
- Merge Since and ReadTs paramenters in the manifest since only one is required. The ReadTs of the current backup will be used as the since timestamp of the next incremental backup. - Remove Manifest field from Request struct. - Separate methods to create backup files and manifest. - Other minor fixes (formatting, documentation, etc).
There was a problem hiding this comment.
Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @golangcibot and @manishrjain)
ee/backup/file_handler.go, line 139 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
bools: redundant or: m.Since == 0 || m.Since == 0 (from
govet)
Done.
ee/backup/s3_handler.go, line 272 at r1 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
bools: redundant or: m.Since == 0 || m.Since == 0 (from
govet)
Done.
There was a problem hiding this comment.
Got a few comments.
Reviewed 6 of 9 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @golangcibot and @martinmr)
ee/backup/backup.go, line 52 at r2 (raw file):
// that should not be inside the Manifest struct since it should not be // recorded in manifest files. type ManifestStatus struct {
How do you decide what goes into MANIFEST and what goes into Status?
Manifest is typically the one file which is supposed to carry all the meta info. See how we do it in Badger.
ee/backup/backup.go, line 78 at r2 (raw file):
} if err := handler.CreateBackupFiles(uri, r); err != nil {
What does CreateBackupFiles do?
ee/backup/file_handler.go, line 66 at r2 (raw file):
// CreateBackupFiles prepares the a path to save backup files and computes the timestamp // from which to start the backup. func (h *fileHandler) CreateBackupFiles(uri *url.URL, req *Request) error {
Can this func take in whatever is at req.Backup and return (*Request, error)? That'd be clearer and seems to the right usage here. That is, this func is responsible for generating the request.
If you do that, then call the func Prepare.
ee/backup/handler.go, line 62 at r2 (raw file):
// creating new backup files at a location described by the URL. The caller of this // comes from an HTTP request. CreateBackupFiles(*url.URL, *Request) error
Call it PrepareForWrite?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @golangcibot and @martinmr)
ee/backup/file_handler.go, line 66 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Can this func take in whatever is at req.Backup and return
(*Request, error)? That'd be clearer and seems to the right usage here. That is, this func is responsible for generating the request.If you do that, then call the func
Prepare.
Or, Setup
- Some renaming. - Reading manifests to read the since timestamp is a separate method. - Manifests are read first, then the request is sent to all groups.
| type handler interface { | ||
| // For all methods below, the URL object is parsed as described in `newHandler' and | ||
| // the Processor object has the DB, estimated tablets size, and backup parameters. | ||
| type UriHandler interface { |
There was a problem hiding this comment.
type UriHandler should be URIHandler (from golint)
| if err != nil { | ||
| return nil, err | ||
| } | ||
| func NewUriHandler(uri *url.URL) (UriHandler, error) { |
There was a problem hiding this comment.
func NewUriHandler should be NewURIHandler (from golint)
There was a problem hiding this comment.
Reviewable status: 0 of 10 files reviewed, 9 unresolved discussions (waiting on @golangcibot, @manishrjain, and @martinmr)
ee/backup/backup.go, line 52 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
How do you decide what goes into MANIFEST and what goes into Status?
Manifest is typically the one file which is supposed to carry all the meta info. See how we do it in Badger.
Done. This type has been removed.
ee/backup/backup.go, line 78 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
What does CreateBackupFiles do?
It creates the backup file object (in case of s3) or file. Then the writer interface is used to write to that file.
ee/backup/file_handler.go, line 66 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Or, Setup
Done. Separated the function into two. One prepares the files for writing and another reads the manifests to get the right timestamp.
ee/backup/handler.go, line 62 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Call it
PrepareForWrite?
I kept CreateBackupFile since I think it's more explicit about what is being created.
There was a problem hiding this comment.
Reviewable status: 0 of 10 files reviewed, 9 unresolved discussions (waiting on @golangcibot, @manishrjain, and @martinmr)
ee/backup/backup.go, line 78 at r2 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
It creates the backup file object (in case of s3) or file. Then the writer interface is used to write to that file.
marking as done.
There was a problem hiding this comment.
Reviewed 10 of 10 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @golangcibot and @martinmr)
required. The ReadTs of the current backup will be used as the since
timestamp of the next incremental backup.
This change is