[pve-devel] [PATCH manager] ui: ceph/Status: fix recovery percentage display

[Thomas Lamprecht]

On 07.07.21 14:30, Dominik Csapak wrote:
> On 7/7/21 2:24 PM, Thomas Lamprecht wrote:
>> On 07.07.21 13:23, Dominik Csapak wrote:
>>> On 7/7/21 12:19 PM, Thomas Lamprecht wrote:
>>>> On 07.07.21 10:47, Dominik Csapak wrote:
>>>>> diff --git a/www/manager6/ceph/Status.js b/www/manager6/ceph/Status.js
>>>>> index e92c698b..52563605 100644
>>>>> --- a/www/manager6/ceph/Status.js
>>>>> +++ b/www/manager6/ceph/Status.js
>>>>> @@ -321,14 +321,14 @@ Ext.define('PVE.node.CephStatus', {
>>>>>        let unhealthy = degraded + unfound + misplaced;
>>>>>        // update recovery
>>>>>        if (pgmap.recovering_objects_per_sec !== undefined || unhealthy > 0) {
>>>>> -        let toRecover = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
>>>>> -        if (toRecover === 0) {
>>>>> +        let totalRecovery = pgmap.misplaced_total || pgmap.unfound_total || pgmap.degraded_total || 0;
>>>>
>>>> why change the variable name, `toRecover` was still OK? Or at least I do not see
>>>> any improvement in making it easier to understand with `totalRecovery` if byte vs.
>>>> objects where a issue of confusion why not addressing that by using `toRecoverObjects`
>>>> or the like
>>> i read the code and thought 'toRecover' means objects that need recovery, but it is not. {misplaced,unfound,degraded}_total each contain
>>> the total number of objects taking part in the recovery
>>> (also the ones that are not unhealthy)
>>>
>>> maybe 'totalRecoveryObjects' would make more sense ?
>>
>> totalRecoveryObjects and toRecoverObjects are so similar that they do not really
>> convey the difference to me for the confusion you had for any other reader, for that
>> I'd rather add a short comment, those tend to be a bit more explicit for subtle stuff.
> 
> ok i'll leave it at 'toRecover' and add a comment what it is in my v2

Adding objects is fine to me though, the basic unit, i.e., size vs. bytes here,
is something that can be encoded in the variable name for dynamic typed languages
like JS - but no hard feelings.

>>
>>>
>>>>
>>>> Also, why not adding those metrics up? If, misplaced and unfound do not have any
>>>> overlap, IIRC, so would def. make sense for those - for degraded I'm not so sure
>>>> about overlap with the other two from top of my head though.
>>>
>>> they contain all the same number
>>> src/mon/PGMap.cc:{467,482,498} pool_sum.stats.sum.num_object_copies
>>
>> ah yeah true, I remember now again. Do you also know where this is actually
>> set (computed).
>>
> 
> no sadly, i tried to check, but i am not so deep into ceph code right now

ok, thanks nonetheless.