-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix --progress flag to properly control progress display and default … #2698
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
Conversation
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.
This looks good to me, once the build warning is fixed!
I think there is a follow up though, for both compression & decompression:
zstd -c --progress /path/to/file > /dev/null
zstd -dc --progress /path/to/file > /dev/null
In both of these cases, no newline is printed at the end of compression. Normally, the progress is replaced with a status line, like these commands:
zstd /path/to/file -o /dev/null
zstd -d /path/to/file -o /dev/null
I think we should either:
- Add a
\n
after the last progress message. - Also print the final status line with
--progress
.
I don't have a super strong preference, but I would go with (2) unless we have an explicit reason why we chose not to.
dfb852e
to
60e2072
Compare
programs/fileio.c
Outdated
@@ -1612,7 +1612,8 @@ FIO_compressFilename_internal(FIO_ctx_t* const fCtx, | |||
U64 const timeLength_ns = UTIL_clockSpanNano(timeStart); | |||
double const timeLength_s = (double)timeLength_ns / 1000000000; | |||
double const cpuLoad_pct = (cpuLoad_s / timeLength_s) * 100; | |||
DISPLAYLEVEL(4, "%-20s : Completed in %.2f sec (cpu load : %.0f%%)\n", | |||
DISPLAYLEVEL((g_display_prefs.progressSetting == FIO_ps_always ? 1 : 4), |
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.
This seems separate from the --progress
notification issue.
This message is more part of the final status.
And I don't believe that this additional information (time spent) belongs to default, which should remain restricted to 1 line.
We may have room for discussion in determining if it's more appropriate for level 3
(-v
) or 4
(-vv
),
but it should certainly not be part of 2
(default).
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.
This was added per Nick's comment suggesting we add the final status line when using --progress to overwrite the last progress message; it will still remain at level 4 in absence of the progress flag
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.
The final status line is just the line specifying the final size and compression ratio.
Information about timing and speed are considered supplementary (and do constitute an additional line).
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.
Ah got it, will update
…progress display on when using -v
…progress display on when using -v