[Freeipa-devel] [PATCH 0350] raise time limit for ldapsearch in upgrade

[Martin Basti]

On 20.11.2015 09:00, Jan Cholasta wrote:
> On 19.11.2015 14:13, Jan Cholasta wrote:
>> On 19.11.2015 14:09, Martin Babinsky wrote:
>>> On 11/19/2015 01:08 PM, Martin Basti wrote:
>>>>
>>>>
>>>> On 18.11.2015 14:26, Martin Basti wrote:
>>>>>
>>>>>
>>>>> On 18.11.2015 14:24, Martin Kosek wrote:
>>>>>> On 11/18/2015 02:18 PM, Martin Basti wrote:
>>>>>>>
>>>>>>> On 18.11.2015 13:55, Martin Kosek wrote:
>>>>>>>> On 11/18/2015 01:30 PM, Martin Basti wrote:
>>>>>>>>> +    DEFAULT_TIME_LIMIT = -1.0
>>>>>>>>> +    DEFAULT_SIZE_LIMIT = 0
>>>>>>>> ...
>>>>>>>>>            if time_limit is None or time_limit == 0:
>>>>>>>>> -            time_limit = -1.0
>>>>>>>>> +            if self.time_limit is not None and self.time_limit
>>>>>>>>> != 0:
>>>>>>>>> +                time_limit = self.time_limit
>>>>>>>>> +            else:
>>>>>>>>> +                time_limit = self.DEFAULT_TIME_LIMIT
>>>>>>>>> +
>>>>>>>> I wonder why is the -1 default time limit a float number, I would
>>>>>>>> expect that
>>>>>>>> some trouble may emerge out of it. Maybe Rob knows?
>>>>>>>>
>>>>>>> Python LDAP allows to have smaller granularity than seconds and it
>>>>>>> internally
>>>>>>> convert time limit values to float.
>>>>>> Hm, ok. I was just curious since the value we expose in API is Int
>>>>>> and the
>>>>>> default does not use the smaller granularity, so I was thinking "Why
>>>>>> bother?".
>>>>>> If it is fixed in your patch, good. If not, good also, no need to
>>>>>> bikeshed here
>>>>>> I suppose.
>>>>> I just keep the original values there, I do not want to touch it with
>>>>> this patch
>>>>>
>>>> Updated patch attached, the original one has wrong default limit for
>>>> ldap2
>>>>
>>>>
>>> ACK
>>
>> Hold your horses.
>
> 1) The upgrade_ldapsearch_limit option is not very useful, either add 
> generic search_time_limit and search_records_limit options, or don't 
> add any options at all (which, incidentally, is exactly what the 
> ticket asks for).
>
> 2) The limits are not passed to the connection using __init__() 
> anywhere, so there is no need to have arguments for them.
>
> 3) Rather than adding layers of if statements everywhere, implement 
> the time_limit and size_limit attributes as properties.
>
> 4) Use config_entry.single_value when setting defaults in 
> ldap2.get_ipa_config().
>
> BTW, add comments only when it's not obvious what the code does - a 
> comment saying "set defaults" above two .setdefault() calls is pretty 
> redundant.
>
Updated patch attached
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0350.3-Upgrade-increase-time-limit-for-upgrades.patch
Type: text/x-patch
Size: 8070 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151124/fe9d006e/attachment.bin>