Re: [pbs-devel] [PATCH proxmox-backup 0/3] fix #5285: log global statistics for sync job

[Max Carrara <m.carrara@proxmox.com>]

On 3/6/24 18:29, Max Carrara wrote:
> On 3/6/24 15:11, Christian Ebner wrote:
>> Adds a global summary of the transferred chunk size and count, as well
>> as the average transfer rate of a sync job to it's task log.
>>
>> Patch 1/3 introduces a PullStats object, used to return the relevant
>> data from each pull related method call.
>>
>> Patch 2/3 adds the summary log line to the tasklog.
>>
>> Patch 3/3 finally adapts the current log output to use the
>> functionality of `HumanByte` to produce consistent output.
>>
>> Tested by creating a local sync job and syncing a datastore, checking
>> the output in the tasklog.
>> Chunk counts where compared to `find .chunks -type f -print | wc -l`.
>>
>> Bugtracker link:
>> https://bugzilla.proxmox.com/show_bug.cgi?id=5285
>>
>> Christian Ebner (3):
>>   server: sync: return `PullStats` for pull related methods
>>   fix #5285: api: sync: add job summary to task log
>>   server: sync: use HumanByte for task log output
>>
>>  src/api2/pull.rs   |  12 ++++-
>>  src/server/pull.rs | 130 ++++++++++++++++++++++++++++++---------------
>>  2 files changed, 99 insertions(+), 43 deletions(-)
>>
> 
> Looks pretty good to me!
> 
> * The patches are very straightforward and easy to follow.
> * Code is formatted with `cargo fmt`.
> * `cargo clippy` doesn't complain about your changes either.
> 
> Unfortunately didn't get around to testing it just yet due to the
> proxmox-schema changes (as you spotted off-list already), so will
> do that as soon as that's sorted out.
> 
> Can't complain otherwise, very clean!
> 
> Reviewed-By: Max Carrara <m.carrara@proxmox.com>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 

So, I just got around to test it as well (by manually disabling checks in
proxmox-schema for the time being).

* Created a new datastore
* Set up a local sync job that pulls from my existing datastore
* Verified number of transferred chunks using `find .chunks -type f -print | wc -l`
  (like you did)

LGTM, looks great! I like the format of the output.


So, in total:

Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>