backupccl: refactor getBackupDetailsAndManifest#78350
backupccl: refactor getBackupDetailsAndManifest#78350craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
1bb4984 to
094e598
Compare
msbutler
left a comment
There was a problem hiding this comment.
overall, I think it's a great refactor! left some initial comments
pkg/ccl/backupccl/backup_planning.go
Outdated
| Destination: details.Destination, | ||
| StartTime: details.StartTime, | ||
| EndTime: details.EndTime, | ||
| URI: details.URI, |
There was a problem hiding this comment.
given all the objects the reader has to keep track of during backup_planning, i don't think it's worth creating another BackupDetails object with less info.
There was a problem hiding this comment.
ok! Wanted to keep the API the same for a pure refactor, but I think you're right. Removed.
f001b30 to
72972fb
Compare
msbutler
left a comment
There was a problem hiding this comment.
LGTM
Before merging, I think it's worth having Aditya take a look.
6b28e74 to
3e93417
Compare
adityamaru
left a comment
There was a problem hiding this comment.
Couple comments but everything else LGTM! Thanks for doing this. Feel free to ignore but if you want you could add a comment or two in the scaffolding method to direct future authors to put things in the right method. Egs: a comment after updateBackupDetails saying that at this point we expect the backup job details to be fully constructed, and will be treated as read-only hereafter.
| @@ -1575,104 +1548,174 @@ func getBackupDetailAndManifest( | |||
|
|
|||
| prevBackups, encryptionOptions, memSize, err := fetchPreviousBackups(ctx, &mem, user, | |||
There was a problem hiding this comment.
It makes me slightly uneasy that the error handling for this error has strayed further away from the assignment. Can we move the error check and defer back or am I missing something?
There was a problem hiding this comment.
Yup, just an oversight. Thanks for catching!
| if err != nil { | ||
| return jobspb.BackupDetails{}, BackupManifest{}, err | ||
| } | ||
| defer defaultStore.Close() |
There was a problem hiding this comment.
don't we still need to defer a close to the store?
There was a problem hiding this comment.
Yes, thank you! Good catch!
a8af0dd to
6312259
Compare
This should be a complete no-op from a functionality perspective. The smaller, more encapsulated pieces should be easier to reason about. Release note: None
6312259 to
14551a9
Compare
|
bors r+ |
|
Build failed (retrying...): |
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build failed (retrying...): |
|
Build succeeded: |
This should be a complete no-op from a functionality perspective.
The smaller, more encapsulated pieces should be easier to reason about.
Specifically, getBackupDetailsAndManifest has now become a scaffolding function, calling the new updateBackupDetails, createBackupManifest, and validateBackupDetailsAndManifest in turn. These functions have a narrower focus and are a bit easier to follow.
It's also now a bit clearer where one would go to, say, add more validation conditions.
Release note: None