[pbs-devel] [PATCH v3 proxmox] time: drop trailing space when not showing seconds at end

[pbs-devel] [PATCH v3 proxmox] time: drop trailing space when not showing seconds at end

[Christian Ebner]

Seconds are not displayed when the value is smaller than 0.1s and
they are not at the start of the display output, e.g. `1h 2m`.
Drop the additional whitespace currently appended for this edge case.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
changes since version 2:
- not present in previous version

 proxmox-time/src/time_span.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/proxmox-time/src/time_span.rs b/proxmox-time/src/time_span.rs
index 2ccdf08b..2f8dd1c1 100644
--- a/proxmox-time/src/time_span.rs
+++ b/proxmox-time/src/time_span.rs
@@ -170,11 +170,11 @@ impl std::fmt::Display for TimeSpan {
                 do_write(self.minutes, "min")?;
             }
         }
-        if !first {
-            write!(f, " ")?;
-        }
         let seconds = self.seconds as f64 + (self.msec as f64 / 1000.0);
         if seconds >= 0.1 {
+            if !first {
+                write!(f, " ")?;
+            }
             if seconds >= 1.0 || !first {
                 write!(f, "{:.0}s", seconds)?;
             } else {
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v3 proxmox-backup] partial fix #5560: client: periodically show backup progress

[Christian Ebner]

Spawn a new tokio task which about every minute displays the
cumulative progress of the backup for pxar, ppxar or img archive
streams. Catalog and metadata archive streams are excluded from the
output for better readability, and because the catalog upload lives
for the whole upload time, leading to possible temporal
misalignments in the output. The actual payload data is written via
the other streams anyway.

Add accounting for uploaded chunks, to distinguish from chunks queued
for upload, but not actually uploaded yet.

Example output in the backup task log:
```
...
INFO:  processed 2.471 GiB in 1min, uploaded 2.439 GiB
INFO:  processed 4.963 GiB in 2min, uploaded 4.929 GiB
INFO:  processed 7.349 GiB in 3min, uploaded 7.284 GiB
...
```

This partially fixes issue 5560:
https://bugzilla.proxmox.com/show_bug.cgi?id=5560

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Changes since version 2, thanks Thomas for comments and Gabriel for testing:
- Clenup log output by reducing to processed bytes, time and uploaded bytes
- Format time in human readable manner using proxmox-time's `TimeSpan`
- Drop all now unused atomic progress counters
- Adapted commit message accordingly

Changes since version 1, thanks Gabriel for comments and testing:
- Abort progress output task when upload stream is finished
- Limit output to pxar, ppxar or img archives for cleaner output
- Adapted commit title and message

 pbs-client/src/backup_writer.rs | 74 ++++++++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 11 deletions(-)

diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index d63c09b5a..4d2e3d08c 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -21,6 +21,7 @@ use pbs_datastore::{CATALOG_NAME, PROXMOX_BACKUP_PROTOCOL_ID_V1};
 use pbs_tools::crypt_config::CryptConfig;
 
 use proxmox_human_byte::HumanByte;
+use proxmox_time::TimeSpan;
 
 use super::inject_reused_chunks::{InjectChunks, InjectReusedChunks, InjectedChunksInfo};
 use super::merge_known_chunks::{MergeKnownChunks, MergedChunkInfo};
@@ -65,7 +66,12 @@ struct UploadStats {
     csum: [u8; 32],
 }
 
-type UploadQueueSender = mpsc::Sender<(MergedChunkInfo, Option<h2::client::ResponseFuture>)>;
+struct ChunkUploadResponse {
+    future: h2::client::ResponseFuture,
+    size: usize,
+}
+
+type UploadQueueSender = mpsc::Sender<(MergedChunkInfo, Option<ChunkUploadResponse>)>;
 type UploadResultReceiver = oneshot::Receiver<Result<(), Error>>;
 
 impl BackupWriter {
@@ -332,6 +338,12 @@ impl BackupWriter {
             .as_u64()
             .unwrap();
 
+        let archive = if log::log_enabled!(log::Level::Debug) {
+            archive_name
+        } else {
+            pbs_tools::format::strip_server_file_extension(archive_name)
+        };
+
         let upload_stats = Self::upload_chunk_info_stream(
             self.h2.clone(),
             wid,
@@ -345,16 +357,12 @@ impl BackupWriter {
             },
             options.compress,
             injections,
+            archive,
         )
         .await?;
 
         let size_dirty = upload_stats.size - upload_stats.size_reused;
         let size: HumanByte = upload_stats.size.into();
-        let archive = if log::log_enabled!(log::Level::Debug) {
-            archive_name
-        } else {
-            pbs_tools::format::strip_server_file_extension(archive_name)
-        };
 
         if upload_stats.chunk_injected > 0 {
             log::info!(
@@ -462,6 +470,7 @@ impl BackupWriter {
         h2: H2Client,
         wid: u64,
         path: String,
+        uploaded: Arc<AtomicUsize>,
     ) -> (UploadQueueSender, UploadResultReceiver) {
         let (verify_queue_tx, verify_queue_rx) = mpsc::channel(64);
         let (verify_result_tx, verify_result_rx) = oneshot::channel();
@@ -470,15 +479,21 @@ impl BackupWriter {
         tokio::spawn(
             ReceiverStream::new(verify_queue_rx)
                 .map(Ok::<_, Error>)
-                .and_then(move |(merged_chunk_info, response): (MergedChunkInfo, Option<h2::client::ResponseFuture>)| {
+                .and_then(move |(merged_chunk_info, response): (MergedChunkInfo, Option<ChunkUploadResponse>)| {
                     match (response, merged_chunk_info) {
                         (Some(response), MergedChunkInfo::Known(list)) => {
                             Either::Left(
                                 response
+                                    .future
                                     .map_err(Error::from)
                                     .and_then(H2Client::h2api_response)
-                                    .and_then(move |_result| {
-                                        future::ok(MergedChunkInfo::Known(list))
+                                    .and_then({
+                                        let uploaded = uploaded.clone();
+                                        move |_result| {
+                                            // account for uploaded bytes for progress output
+                                            uploaded.fetch_add(response.size, Ordering::SeqCst);
+                                            future::ok(MergedChunkInfo::Known(list))
+                                        }
                                     })
                             )
                         }
@@ -636,6 +651,7 @@ impl BackupWriter {
         crypt_config: Option<Arc<CryptConfig>>,
         compress: bool,
         injections: Option<std::sync::mpsc::Receiver<InjectChunks>>,
+        archive: &str,
     ) -> impl Future<Output = Result<UploadStats, Error>> {
         let total_chunks = Arc::new(AtomicUsize::new(0));
         let total_chunks2 = total_chunks.clone();
@@ -646,25 +662,51 @@ impl BackupWriter {
 
         let stream_len = Arc::new(AtomicUsize::new(0));
         let stream_len2 = stream_len.clone();
+        let stream_len3 = stream_len.clone();
         let compressed_stream_len = Arc::new(AtomicU64::new(0));
         let compressed_stream_len2 = compressed_stream_len.clone();
         let reused_len = Arc::new(AtomicUsize::new(0));
         let reused_len2 = reused_len.clone();
         let injected_len = Arc::new(AtomicUsize::new(0));
         let injected_len2 = injected_len.clone();
+        let uploaded_len = Arc::new(AtomicUsize::new(0));
 
         let append_chunk_path = format!("{}_index", prefix);
         let upload_chunk_path = format!("{}_chunk", prefix);
         let is_fixed_chunk_size = prefix == "fixed";
 
         let (upload_queue, upload_result) =
-            Self::append_chunk_queue(h2.clone(), wid, append_chunk_path);
+            Self::append_chunk_queue(h2.clone(), wid, append_chunk_path, uploaded_len.clone());
 
         let start_time = std::time::Instant::now();
 
         let index_csum = Arc::new(Mutex::new(Some(openssl::sha::Sha256::new())));
         let index_csum_2 = index_csum.clone();
 
+        let progress_handle = if archive.ends_with(".img")
+            || archive.ends_with(".pxar")
+            || archive.ends_with(".ppxar")
+        {
+            Some(tokio::spawn(async move {
+                loop {
+                    tokio::time::sleep(tokio::time::Duration::from_secs(60)).await;
+
+                    let size = stream_len3.load(Ordering::SeqCst);
+                    let size_uploaded = uploaded_len.load(Ordering::SeqCst);
+                    let elapsed = start_time.elapsed();
+
+                    log::info!(
+                        " processed {} in {}, uploaded {}",
+                        HumanByte::from(size),
+                        TimeSpan::from(elapsed),
+                        HumanByte::from(size_uploaded),
+                    );
+                }
+            }))
+        } else {
+            None
+        };
+
         stream
             .inject_reused_chunks(injections, stream_len.clone())
             .and_then(move |chunk_info| match chunk_info {
@@ -776,7 +818,13 @@ impl BackupWriter {
                     Either::Left(h2.send_request(request, upload_data).and_then(
                         move |response| async move {
                             upload_queue
-                                .send((new_info, Some(response)))
+                                .send((
+                                    new_info,
+                                    Some(ChunkUploadResponse {
+                                        future: response,
+                                        size: chunk_info.chunk_len as usize,
+                                    }),
+                                ))
                                 .await
                                 .map_err(|err| {
                                     format_err!("failed to send to upload queue: {}", err)
@@ -806,6 +854,10 @@ impl BackupWriter {
                 let mut guard = index_csum_2.lock().unwrap();
                 let csum = guard.take().unwrap().finish();
 
+                if let Some(handle) = progress_handle {
+                    handle.abort();
+                }
+
                 futures::future::ok(UploadStats {
                     chunk_count,
                     chunk_reused,
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied: [PATCH v3 proxmox-backup] partial fix #5560: client: periodically show backup progress

[Thomas Lamprecht]

Am 11/10/2024 um 11:33 schrieb Christian Ebner:
> Spawn a new tokio task which about every minute displays the
> cumulative progress of the backup for pxar, ppxar or img archive
> streams. Catalog and metadata archive streams are excluded from the
> output for better readability, and because the catalog upload lives
> for the whole upload time, leading to possible temporal
> misalignments in the output. The actual payload data is written via
> the other streams anyway.
> 
> Add accounting for uploaded chunks, to distinguish from chunks queued
> for upload, but not actually uploaded yet.
> 
> Example output in the backup task log:
> ```
> ...
> INFO:  processed 2.471 GiB in 1min, uploaded 2.439 GiB
> INFO:  processed 4.963 GiB in 2min, uploaded 4.929 GiB
> INFO:  processed 7.349 GiB in 3min, uploaded 7.284 GiB
> ...
> ```
> 
> This partially fixes issue 5560:
> https://bugzilla.proxmox.com/show_bug.cgi?id=5560
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> Changes since version 2, thanks Thomas for comments and Gabriel for testing:
> - Clenup log output by reducing to processed bytes, time and uploaded bytes
> - Format time in human readable manner using proxmox-time's `TimeSpan`
> - Drop all now unused atomic progress counters
> - Adapted commit message accordingly
> 
> Changes since version 1, thanks Gabriel for comments and testing:
> - Abort progress output task when upload stream is finished
> - Limit output to pxar, ppxar or img archives for cleaner output
> - Adapted commit title and message
> 
>  pbs-client/src/backup_writer.rs | 74 ++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 11 deletions(-)
> 
>

applied, thanks! With some opinionated line-reduction as follow-up (opinionated
because it was OK as is too, and I could have noticed that on the previous
review, sorry...)

Did not think it through at all, but maybe it makes sense to add the uploaded
size also to the upload stats.

And FWIW, I slightly changed my mind and now think that having the reused
counter also printed might indeed be nice, but as mentioned in the v2 reply,
we can always extend this and starting out simpler is OK – if users directly
request this you get a free told-you-so coupon applicable to me directly
though ;-P


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] applied: [PATCH v3 proxmox-backup] partial fix #5560: client: periodically show backup progress

[Christian Ebner]

On 10/17/24 17:02, Thomas Lamprecht wrote:
> applied, thanks! With some opinionated line-reduction as follow-up (opinionated
> because it was OK as is too, and I could have noticed that on the previous
> review, sorry...)
> 
> Did not think it through at all, but maybe it makes sense to add the uploaded
> size also to the upload stats.

For now I do not see much benefit for this, as after consuming the full 
upload stream there are no more chunks queued for upload, so the 
counters already present in `UploadStats` contain the same information.

And this information is already shown to the user at the end of the 
upload as, e.g.:
```
had to backup 1.66 GiB of 285.242 GiB (compressed 1.274 GiB) in 58.52 s 
(average 29.053 MiB/s)
```

> And FWIW, I slightly changed my mind and now think that having the reused
> counter also printed might indeed be nice, but as mentioned in the v2 reply,
> we can always extend this and starting out simpler is OK – if users directly
> request this you get a free told-you-so coupon applicable to me directly
> though ;-P

;)



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] applied: [PATCH v3 proxmox-backup] partial fix #5560: client: periodically show backup progress

[Thomas Lamprecht]

Am 17/10/2024 um 17:53 schrieb Christian Ebner:
> On 10/17/24 17:02, Thomas Lamprecht wrote:
>> applied, thanks! With some opinionated line-reduction as follow-up (opinionated
>> because it was OK as is too, and I could have noticed that on the previous
>> review, sorry...)
>>
>> Did not think it through at all, but maybe it makes sense to add the uploaded
>> size also to the upload stats.
> 
> For now I do not see much benefit for this, as after consuming the full 
> upload stream there are no more chunks queued for upload, so the 
> counters already present in `UploadStats` contain the same information.
> 
> And this information is already shown to the user at the end of the 
> upload as, e.g.:
> ```
> had to backup 1.66 GiB of 285.242 GiB (compressed 1.274 GiB) in 58.52 s 
> (average 29.053 MiB/s)
> ```

Ack, thanks for clearing this up to me.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] applied: [PATCH v3 proxmox-backup] partial fix #5560: client: periodically show backup progress

[Christian Ebner]

On 10/18/24 09:20, Thomas Lamprecht wrote:
> Am 17/10/2024 um 17:53 schrieb Christian Ebner:
>> On 10/17/24 17:02, Thomas Lamprecht wrote:
>>> applied, thanks! With some opinionated line-reduction as follow-up (opinionated
>>> because it was OK as is too, and I could have noticed that on the previous
>>> review, sorry...)
>>>
>>> Did not think it through at all, but maybe it makes sense to add the uploaded
>>> size also to the upload stats.
>>
>> For now I do not see much benefit for this, as after consuming the full
>> upload stream there are no more chunks queued for upload, so the
>> counters already present in `UploadStats` contain the same information.
>>
>> And this information is already shown to the user at the end of the
>> upload as, e.g.:
>> ```
>> had to backup 1.66 GiB of 285.242 GiB (compressed 1.274 GiB) in 58.52 s
>> (average 29.053 MiB/s)
>> ```
> 
> Ack, thanks for clearing this up to me.

One thing still came to mind yesterday though:
Progress logging is now enabled unconditionally, and as is it is not 
possible to disable it or configure the interval.

So I plan to send a patch to make the progress log output opt-in, and 
make the interval configurable within a meaningful value range (maybe 
within 0s to 3600s?).

Something like:
```
proxmox-backup-client backup root.pxar:/ --progress-log-interval=10
```

And disable progress log output for a value of 0 or the optional 
parameter is not given.

That would also allow to more flexibly control the behavior when being 
invoked by vzdump.

What are your opinions on that?


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] applied: [PATCH v3 proxmox-backup] partial fix #5560: client: periodically show backup progress

[Thomas Lamprecht]

Am 18/10/2024 um 09:37 schrieb Christian Ebner:
> One thing still came to mind yesterday though:
> Progress logging is now enabled unconditionally, and as is it is not 
> possible to disable it or configure the interval.
> 
> So I plan to send a patch to make the progress log output opt-in, and 

Personally I'd prefer opt-out, I'm not a fan of tools that stay silent
by default (looking at you `dd`).

Tools can always turn this off easily, and we already have some output
so it's not like we'd previously have none and now suddenly print stuff,
which might indeed surprise some existing users that relied on no output
(IMO a bit weird thing to do, but well not completely out of the picture)

> make the interval configurable within a meaningful value range (maybe 
> within 0s to 3600s?).
> 
> Something like:
> ```
> proxmox-backup-client backup root.pxar:/ --progress-log-interval=10
> ```
> 
> And disable progress log output for a value of 0 or the optional 
> parameter is not given.
> 
> That would also allow to more flexibly control the behavior when being 
> invoked by vzdump.
> 
> What are your opinions on that?

Minus the s/opt-in/opt-out/ preference that's fine by me. We could then
also switch to a higher reporting rate in PVE, albeit FWIW, for VM backups
there we use 10s and then slow down after some time, trying to take a
balance for early fast feedback and short backups and long backups.
And just throwing out ideas, one could also do a size-related interval,
like print a report every (at least) X MB or GB uploaded.

Oh, and do you plan to take a TimeSpan as interval parameter? Might
provide nice UX here.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] applied: [PATCH v3 proxmox-backup] partial fix #5560: client: periodically show backup progress

[Christian Ebner]

On 10/18/24 09:53, Thomas Lamprecht wrote:
> Am 18/10/2024 um 09:37 schrieb Christian Ebner:
>> One thing still came to mind yesterday though:
>> Progress logging is now enabled unconditionally, and as is it is not
>> possible to disable it or configure the interval.
>>
>> So I plan to send a patch to make the progress log output opt-in, and
> 
> Personally I'd prefer opt-out, I'm not a fan of tools that stay silent
> by default (looking at you `dd`).
> 
> Tools can always turn this off easily, and we already have some output
> so it's not like we'd previously have none and now suddenly print stuff,
> which might indeed surprise some existing users that relied on no output
> (IMO a bit weird thing to do, but well not completely out of the picture)

Ack'ed, so let's keep the current default progress log output interval 
of 60 seconds and make it opt-out by allowing to set the interval to 0.
Disabling should however be possible, as the backup upload writer stream 
might be used in other places as well, having unintentional side effects 
(e.g. the sync job in push direction will use the same code path).

> 
>> make the interval configurable within a meaningful value range (maybe
>> within 0s to 3600s?).
>>
>> Something like:
>> ```
>> proxmox-backup-client backup root.pxar:/ --progress-log-interval=10
>> ```
>>
>> And disable progress log output for a value of 0 or the optional
>> parameter is not given.
>>
>> That would also allow to more flexibly control the behavior when being
>> invoked by vzdump.
>>
>> What are your opinions on that?
> 
> Minus the s/opt-in/opt-out/ preference that's fine by me. We could then
> also switch to a higher reporting rate in PVE, albeit FWIW, for VM backups
> there we use 10s and then slow down after some time, trying to take a
> balance for early fast feedback and short backups and long backups.
> And just throwing out ideas, one could also do a size-related interval,
> like print a report every (at least) X MB or GB uploaded.

Hmm, that would be nice to have, maybe allow to pass either a `TimeSpan` 
or a `HumanByte` for the log interval parameter? Not sure if parsing 
would work without possible value clashes though (e.g. 4M could be 4 
months or 4 MiB...).

But maybe that is overkill and not so user friendly, the better option 
being to set either time or size based intervals by (yet another) flag.

> Oh, and do you plan to take a TimeSpan as interval parameter? Might
> provide nice UX here.

That is a great idea!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] applied: [PATCH v3 proxmox-backup] partial fix #5560: client: periodically show backup progress

[Thomas Lamprecht]

Am 18/10/2024 um 10:33 schrieb Christian Ebner:
> On 10/18/24 09:53, Thomas Lamprecht wrote:
>> Minus the s/opt-in/opt-out/ preference that's fine by me. We could then
>> also switch to a higher reporting rate in PVE, albeit FWIW, for VM backups
>> there we use 10s and then slow down after some time, trying to take a
>> balance for early fast feedback and short backups and long backups.
>> And just throwing out ideas, one could also do a size-related interval,
>> like print a report every (at least) X MB or GB uploaded.
> 
> Hmm, that would be nice to have, maybe allow to pass either a `TimeSpan` 
> or a `HumanByte` for the log interval parameter? Not sure if parsing 
> would work without possible value clashes though (e.g. 4M could be 4 
> months or 4 MiB...).
> 
> But maybe that is overkill and not so user friendly, the better option 
> being to set either time or size based intervals by (yet another) flag.
> 

Yeah, if we do this I'd probably separate it by type, as while it might
work out for this specific one mixing units can get confusing fast in
general.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied: [PATCH v3 proxmox] time: drop trailing space when not showing seconds at end

[Thomas Lamprecht]

Am 11/10/2024 um 11:33 schrieb Christian Ebner:
> Seconds are not displayed when the value is smaller than 0.1s and
> they are not at the start of the display output, e.g. `1h 2m`.
> Drop the additional whitespace currently appended for this edge case.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 2:
> - not present in previous version
> 
>  proxmox-time/src/time_span.rs | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
>

applied this one, thanks!

FYI: I found a bug within the Display code, months got printed with the
"m" unit, which is the unit for minutes. Besides fixing that I also made
it print "m" for minutes now to have more consistent unit variants (all
single letter) see the commit message for more details.

I also added a basic module documentation to convey what the format is and
where it comes from and some basic unit tests, could be surely expanded
though.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] applied: [PATCH v3 proxmox] time: drop trailing space when not showing seconds at end

[Christian Ebner]

On 10/17/24 15:16, Thomas Lamprecht wrote:
> applied this one, thanks!
> 
> FYI: I found a bug within the Display code, months got printed with the
> "m" unit, which is the unit for minutes. Besides fixing that I also made
> it print "m" for minutes now to have more consistent unit variants (all
> single letter) see the commit message for more details.
> 
> I also added a basic module documentation to convey what the format is and
> where it comes from and some basic unit tests, could be surely expanded
> though.

Great! So should we maybe start to switch over time duration output in 
the backup client logs to `TimeSpan`s display output?
There are a few places where purely seconds are used.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] applied: [PATCH v3 proxmox] time: drop trailing space when not showing seconds at end

[Thomas Lamprecht]

Am 17/10/2024 um 16:40 schrieb Christian Ebner:
> On 10/17/24 15:16, Thomas Lamprecht wrote:
>> applied this one, thanks!
>>
>> FYI: I found a bug within the Display code, months got printed with the
>> "m" unit, which is the unit for minutes. Besides fixing that I also made
>> it print "m" for minutes now to have more consistent unit variants (all
>> single letter) see the commit message for more details.
>>
>> I also added a basic module documentation to convey what the format is and
>> where it comes from and some basic unit tests, could be surely expanded
>> though.
> 
> Great! So should we maybe start to switch over time duration output in 
> the backup client logs to `TimeSpan`s display output?
> There are a few places where purely seconds are used.

Hmm, not sure if I would unconditionally recommend it, but could be fine
for most places.

Albeit, for things where humans are not the primary consumer it might be
good to allow controlling this, potentially a --no-human-output flag that
causes things like TimeSpan and HumanByte to print just second/byte numbers.
But it also seems like a lot of work to do if there isn't even someone
requesting that use case.


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel