[Kimchi-devel] [PATCH V2] Sort device paths shown when creating a logical storage pool

[Rodrigo Trujillo]

On 04/02/2014 05:30 AM, Zhou Zheng Sheng wrote:
> on 2014/04/02 03:25, Aline Manera wrote:
>> On 04/01/2014 04:21 PM, Rodrigo Trujillo wrote:
>>> On 04/01/2014 03:13 PM, Aline Manera wrote:
>>>> On 03/27/2014 11:12 PM, Rodrigo Trujillo wrote:
>>>>> This patch sorts the host partitions list returned by backend by
>>>>> partition path. Then UI is going to show paths sorted, improving
>>>>> the user experience.
>>>>>
>>>>> Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo at linux.vnet.ibm.com>
>>>>> ---
>>>>>    src/kimchi/control/host.py | 20 +++++++++++++++++++-
>>>>>    1 file changed, 19 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/kimchi/control/host.py b/src/kimchi/control/host.py
>>>>> index cfc24bd..d4387f4 100644
>>>>> --- a/src/kimchi/control/host.py
>>>>> +++ b/src/kimchi/control/host.py
>>>>> @@ -20,7 +20,7 @@
>>>>>    import cherrypy
>>>>>
>>>>>    from kimchi.control.base import Collection, Resource
>>>>> -from kimchi.control.utils import UrlSubNode, validate_method
>>>>> +from kimchi.control.utils import UrlSubNode, validate_method, model_fn
>>>>>    from kimchi.exception import OperationFailed
>>>>>    from kimchi.template import render
>>>>>
>>>>> @@ -64,6 +64,24 @@ class Partitions(Collection):
>>>>>            super(Partitions, self).__init__(model)
>>>>>            self.resource = Partition
>>>>>
>>>>> +    # Defining get_resources in order to return list of partitions
>>>>> in UI
>>>>> +    # sorted by their path
>>>>> +    def _get_resources(self, flag_filter):
>>>>> +        try:
>>>>> +            get_list = getattr(self.model, model_fn(self, 'get_list'))
>>>>> +            idents = get_list(*self.model_args, **flag_filter)
>>>>> +            res_list = []
>>>>> +            for ident in idents:
>>>>> +                # internal text, get_list changes ident to unicode
>>>>> for sorted
>>>>> +                args = self.resource_args + [ident]
>>>>> +                res = self.resource(self.model, *args)
>>>>> +                res.lookup()
>>>>> +                res_list.append(res)
>>>>> +            # Sort by partition path
>>>>> +            res_list.sort(key=lambda x: x.info['path'])
>>>>> +            return res_list
>>>>> +        except AttributeError:
>>>>> +            return []
>>>> Wow! So you will get all resources for then sort them?
>>>> Why don't change Partitions.get_list() to return the sorted list?
>>>> get_partitions_names
>>>> If you check disks.get_partitions_names() it already contains the
>>>> device path, you just need to sort
>>>> the list before returning
>>>>
>>>> So disks.get_partitions_names() will return the list sorted by path.
>>> I do not see path in disks.get_partitions_names() ... and If I do this
>>> I would have to sort a bigger list than in _get_resources().
>> The disks.get_partitions_name() is what provide the result for get() so
>> the list is not bigger,
>>
>> Take a look at disks.py line 146
>>
> Sorting is for user friendly presentation which is the responsibility of
> the controller or even the front-end.
>
> Considering the sorting efficiency, I think the object size of the
> element in the list can be ignored because the list element might be
> just a reference or a pointer to the real object. The significant factor
> of the sorting time and memory is the length of the list. In this case,
> the length does not change. So there is no big efficiency overhead to
> put it in controller. A valid reason to put the sorting in
> disks.get_partitions_name() is that the sorting code is re-used by
> anyone calling disks.get_partitions_name(). But actually we are not sure
> if the caller needs a sorted list.
>
> Sorting in either controller or disks.get_partitions_name() is OK to me,
> while the former is better.
>
Please, see V4


>>> Please, see the version V4 of this patch ... I think it has the best
>>> approach to solve the problem.
>>>
>>>
>>>>>    class Partition(Resource):
>>>>>        def __init__(self, model, id):
>> _______________________________________________
>> Kimchi-devel mailing list
>> Kimchi-devel at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>
>