-
Notifications
You must be signed in to change notification settings - Fork 111
feat: Progress Update on non-interactive runs (#1395) #1620
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
feat: Progress Update on non-interactive runs (#1395) #1620
Conversation
|
Hi @ikhyunAn I have some general remarks:
Can you try to overwork the PR? Please don't hesitate to ask if you need some (more) guidance! |
|
Hi @aawsome , thank you for the detailed feedback regarding the PR. Here's a crisp breakdown of the refactoring which now uses PR Overwork
FixesContextFollowing up on @aawsome 's general remarks, the latest commit introduces a complete refactoring of the Key Changes1. Refactoring
Addressing the remarks on using
2. GeneralizationThe logging logic supports all progress types - 3. Unified Factory Patternlines 341-347 4. Defaults
TestingValidated against the following scenarios:
|
aawsome
left a comment
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.
Looks mostly fine for me, but I have a few nitpicks ;-)
aawsome
left a comment
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! Thanks @ikhyunAn
Progress Update on non-interactive runs (#1395)
feat(cli): add periodic logging for progress in non-interactive terminalsPR Description
Fixes
#1395
Problem
When running
rusticin headless environments (systemd), the progress bar is hidden, resulting in zero output for long-running operations. This makes it impossible to distinguish between a hang and a working backup.Solution
This PR modifies
ProgressBarsto detect ifstderris a TTY.indicatifanimated progress bars.PeriodicLogmode that prints a plain text status update to stderr at a fixed interval (user-provided, otherwise default to 10s.Changes
PeriodicLogvariant toProgressTypeenum.IsTerminalcheck inprogress_bytes.inc()to check the elapsed time and print a log line if the interval has passed.--progress-intervalis provided, non-interactive mode defaults to 10s to avoid flooding logs (unlike the 100ms default for TTYs).Testing
cargo run ... 2>&1 | cat) produces heartbeat logs:Foods for thought
How
set_lengthworks in rustic_coreDuring backup operations: rustic_core intentionally runs the size calculation in a parallel thread to avoid blocking the actual backup process:
This means:
inc() is called before
set_length()during early progress updates.set_length()is called later once the size scan completes.self.0.length() returns
Noneuntilset_length()is calledCurrent Fix (Graceful handling):
Handle the case where length isn't known yet by checking if let Some(len) = self.0.length() instead of using unwrap_or(0), and display progress without total when unknown.
What needs to be tested:
Given my machine's HW limits (tested backup on 5GB), I haven't been able to run backup on files large enough for
set_lengthto be called. Can someone test on a very large backup task to see ifset_lengthis run and the total size is printed correctly?