Re: [pbs-devel] [PATCH proxmox-backup v2] client/pull: log snapshots that are skipped because of time

Re: [pbs-devel] [PATCH proxmox-backup v2] client/pull: log snapshots that are skipped because of time

[Dietmar Maurer]

> +impl std::fmt::Display for SkipInfo {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        if self.count > 1 {
> +            write!(
> +                f,
> +                "{} snapshots ({}..{}) that are older than the newest local snapshot",
> +                self.count,
> +                proxmox::tools::time::epoch_to_rfc3339_utc(self.oldest)
> +                    .map_err(|_| std::fmt::Error)?,
> +                proxmox::tools::time::epoch_to_rfc3339_utc(self.newest)
> +                    .map_err(|_| std::fmt::Error)?,
> +            )

what is the purpose of this complex message (why we want to show self.oldest and self.newest)?
Its confusing me more than it helps...

> +        } else if self.count == 1 {
> +            write!(
> +                f,
> +                "1 snapshot ({}) that is older than the newest local snapshot",
> +                proxmox::tools::time::epoch_to_rfc3339_utc(self.oldest)
> +                    .map_err(|_| std::fmt::Error)?,
> +            )

do we really need this special case?

> +        } else {
> +            write!(f, "0 snapshots")

Instead, I would avoid to call this function if count is 0 ...

> +        }
> +    }
> +}
> +
>  pub async fn pull_group(

Re: [pbs-devel] [PATCH proxmox-backup v2] client/pull: log snapshots that are skipped because of time

[Dominik Csapak]

On 6/4/21 15:54, Dietmar Maurer wrote:
> 
>> +impl std::fmt::Display for SkipInfo {
>> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>> +        if self.count > 1 {
>> +            write!(
>> +                f,
>> +                "{} snapshots ({}..{}) that are older than the newest local snapshot",
>> +                self.count,
>> +                proxmox::tools::time::epoch_to_rfc3339_utc(self.oldest)
>> +                    .map_err(|_| std::fmt::Error)?,
>> +                proxmox::tools::time::epoch_to_rfc3339_utc(self.newest)
>> +                    .map_err(|_| std::fmt::Error)?,
>> +            )
> 
> what is the purpose of this complex message (why we want to show self.oldest and self.newest)?
> Its confusing me more than it helps...

was the suggestion from Fabian, and i did like the idea to tell the user
*which* snapshots were skipped (and if we only have one line,
there is not many ways to represent that)

> 
>> +        } else if self.count == 1 {
>> +            write!(
>> +                f,
>> +                "1 snapshot ({}) that is older than the newest local snapshot",
>> +                proxmox::tools::time::epoch_to_rfc3339_utc(self.oldest)
>> +                    .map_err(|_| std::fmt::Error)?,
>> +            )
> 
> do we really need this special case?

if we want to keep the info which snapshots are skipped, then
yes imho, otherwise we have line such as

1 snapshots (X..X) that are older than the newest local snapshot

which is grammatically wrong (1 snapshots are older)
and contains redundant info (X..X)


> 
>> +        } else {
>> +            write!(f, "0 snapshots")
> 
> Instead, I would avoid to call this function if count is 0 ...

i already avoid it calling below, but i wanted to implement this,
in case we reuse that struct somewhere else

> 
>> +        }
>> +    }
>> +}
>> +
>>   pub async fn pull_group(

[pbs-devel] [PATCH proxmox-backup v2] client/pull: log snapshots that are skipped because of time

[Dominik Csapak]

we skip snapshots that are older than the newest snapshot of the group in
the target datastore, log it so the user can know why it is not synced

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* condense display trait by
  - omit 0 case (we check fot it anyway)
  - combine other cases by simplifying language (drop 'that is/are',
    change to 'snapshot(s)')

 src/client/pull.rs | 57 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/src/client/pull.rs b/src/client/pull.rs
index 95720973..1ee0e0d1 100644
--- a/src/client/pull.rs
+++ b/src/client/pull.rs
@@ -14,6 +14,7 @@ use crate::{
     backup::*,
     client::*,
     server::WorkerTask,
+    task_log,
     tools::{compute_file_csum, ParallelHandler},
 };
 use proxmox::api::error::{HttpError, StatusCode};
@@ -443,6 +444,51 @@ pub async fn pull_snapshot_from(
     Ok(())
 }
 
+struct SkipInfo {
+    oldest: i64,
+    newest: i64,
+    count: u64,
+}
+
+impl SkipInfo {
+    fn update(&mut self, backup_time: i64) {
+        self.count += 1;
+
+        if backup_time < self.oldest {
+            self.oldest = backup_time;
+        }
+
+        if backup_time > self.newest {
+            self.newest = backup_time;
+        }
+    }
+
+    fn affected(&self) -> Result<String, Error> {
+        match self.count {
+            0 => Ok(String::new()),
+            1 => proxmox::tools::time::epoch_to_rfc3339_utc(self.oldest),
+            _ => {
+                Ok(format!(
+                    "{} .. {}",
+                    proxmox::tools::time::epoch_to_rfc3339_utc(self.oldest)?,
+                    proxmox::tools::time::epoch_to_rfc3339_utc(self.newest)?,
+                ))
+            }
+        }
+    }
+}
+
+impl std::fmt::Display for SkipInfo {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(
+            f,
+            "skipped: {} snapshot(s) ({}) older than the newest local snapshot",
+            self.count,
+            self.affected().map_err(|_| std::fmt::Error)?
+        )
+    }
+}
+
 pub async fn pull_group(
     worker: &WorkerTask,
     client: &HttpClient,
@@ -477,6 +523,12 @@ pub async fn pull_group(
 
     progress.group_snapshots = list.len() as u64;
 
+    let mut skip_info = SkipInfo {
+        oldest: i64::MAX,
+        newest: i64::MIN,
+        count: 0,
+    };
+
     for (pos, item) in list.into_iter().enumerate() {
         let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time)?;
 
@@ -495,6 +547,7 @@ pub async fn pull_group(
 
         if let Some(last_sync_time) = last_sync {
             if last_sync_time > backup_time {
+                skip_info.update(backup_time);
                 continue;
             }
         }
@@ -552,6 +605,10 @@ pub async fn pull_group(
         }
     }
 
+    if skip_info.count > 0 {
+        task_log!(worker, "{}", skip_info);
+    }
+
     Ok(())
 }
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2] client/pull: log snapshots that are skipped because of time

[Dominik Csapak]

we skip snapshots that are older than the newest snapshot of the group in
the target datastore, log it so the user can know why it is not synced

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* only log 1 line per backup group by implementing a 'SkipInfo' struct
  that counts, saves the corresponding backup_times to log, and
  implements 'Display' to log

 src/client/pull.rs | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/src/client/pull.rs b/src/client/pull.rs
index 95720973..a52f4e02 100644
--- a/src/client/pull.rs
+++ b/src/client/pull.rs
@@ -14,6 +14,7 @@ use crate::{
     backup::*,
     client::*,
     server::WorkerTask,
+    task_log,
     tools::{compute_file_csum, ParallelHandler},
 };
 use proxmox::api::error::{HttpError, StatusCode};
@@ -443,6 +444,51 @@ pub async fn pull_snapshot_from(
     Ok(())
 }
 
+struct SkipInfo {
+    oldest: i64,
+    newest: i64,
+    count: u64,
+}
+
+impl SkipInfo {
+    fn update(&mut self, backup_time: i64) {
+        self.count += 1;
+
+        if backup_time < self.oldest {
+            self.oldest = backup_time;
+        }
+
+        if backup_time > self.newest {
+            self.newest = backup_time;
+        }
+    }
+}
+
+impl std::fmt::Display for SkipInfo {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        if self.count > 1 {
+            write!(
+                f,
+                "{} snapshots ({}..{}) that are older than the newest local snapshot",
+                self.count,
+                proxmox::tools::time::epoch_to_rfc3339_utc(self.oldest)
+                    .map_err(|_| std::fmt::Error)?,
+                proxmox::tools::time::epoch_to_rfc3339_utc(self.newest)
+                    .map_err(|_| std::fmt::Error)?,
+            )
+        } else if self.count == 1 {
+            write!(
+                f,
+                "1 snapshot ({}) that is older than the newest local snapshot",
+                proxmox::tools::time::epoch_to_rfc3339_utc(self.oldest)
+                    .map_err(|_| std::fmt::Error)?,
+            )
+        } else {
+            write!(f, "0 snapshots")
+        }
+    }
+}
+
 pub async fn pull_group(
     worker: &WorkerTask,
     client: &HttpClient,
@@ -477,6 +523,12 @@ pub async fn pull_group(
 
     progress.group_snapshots = list.len() as u64;
 
+    let mut skip_info = SkipInfo {
+        oldest: i64::MAX,
+        newest: i64::MIN,
+        count: 0,
+    };
+
     for (pos, item) in list.into_iter().enumerate() {
         let snapshot = BackupDir::new(item.backup_type, item.backup_id, item.backup_time)?;
 
@@ -495,6 +547,7 @@ pub async fn pull_group(
 
         if let Some(last_sync_time) = last_sync {
             if last_sync_time > backup_time {
+                skip_info.update(backup_time);
                 continue;
             }
         }
@@ -552,6 +605,14 @@ pub async fn pull_group(
         }
     }
 
+    if skip_info.count > 0 {
+        task_log!(
+            worker,
+            "skipped: {}",
+            skip_info,
+        );
+    }
+
     Ok(())
 }
 
-- 
2.20.1