-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor backup progress #3264
Refactor backup progress #3264
Conversation
1478439
to
dc064eb
Compare
I don't agree with omitting "archive" or "backup" from the names of the moved types, which are all specific to the backup command. The generic names ui.Counter and ui.Progress mentally conflict with ui/progress.Counter. If this must be moved out of cmd/restic, I suggest it be moved to a new package, maybe archiver/ui, ui/archiver or ui/backup. |
Thanks for you comment, I'm not sure I follow what you're saying. I renamed the interface so that I could re-use the logic of the progress reporting with restores (I haven't submitted that change yet, it depends on this refactor). |
dc064eb
to
af9fe23
Compare
af9fe23
to
a57bf29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amozoss
I've rebased the commit onto the current master. The main change during the rebase is that there's now a SetDryRun()
method.
Besides that I've made a few other noteworthy changes:
- The JSON statistics part of the summary output is restored, which was lost during the refactoring.
- I've removed the unused
TotalErrors
field from theSummary
struct. backup --json | cat -
no longer printed status updates. I've fixed that regression. Now onlybackup --json --quiet
disabled the status updates. The latter actually is a change compared to the current behavior wherebackup --json --quiet
andbackup --json
behave similarly.
The Counter
struct had also diverged between the text and JSON output.
Regarding the name of the Counter
and the Summary
struct I agree with greatroar that these are pretty generic and could be confusing. The cleanest way would probably to move everything, that is the backup.go files and progress.go, into a package internal/ui/backup
. However, as you've mentioned that you want to reuse some part of the logic for restore
, I'm wondering whether some other structure would be preferable.
Move the shared logic into the progress Allows logic to be shared with forth coming restore status
After the refactoring status updates were no longer printed in quiet mode or when the output is not an interactive terminal. However, the JSON output is often piped to e.g. another program. Thus, don't set the update frequency to 0 in that case. The status updates are still disabled for backup --quiet. This also reduces the status update frequency to 60fps compared to a potentially much higher value before the refactoring.
a57bf29
to
503d4c3
Compare
9c1f3ab
to
fbb0e64
Compare
I've added a changelog that his PR also fixes #2738. @greatroar The backup ui code is for now consolidated in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
What does this PR change? What problem does it solve?
ui/backup.go
andui/json/backup.go
are almost identical to one another (copy paste).This change refactors the shared logic into
progress.go
.ui/backup.go
andui/json/backup.go
have already begun to diverge, for examplep.summary.ProcessedBytes += current.Size
was done outside of the lock in json/backup.go , but ui/backup.go had the aboveThis will continue to be a "hope we don't forget to update the other place" problem..
This change also allows the progress reporting/printing to be shared with restore status (forth coming after this is merged).
Was the change discussed in an issue or in the forum before?
No it wasn't
Checklist
changelog/unreleased/
that describes the changes for our users (template here)gofmt
on the code in all commits