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

[Jan Cholasta]

On 30.11.2015 14:15, Martin Basti wrote:
>
>
> On 30.11.2015 08:19, Jan Cholasta wrote:
>> On 24.11.2015 10:15, Martin Basti wrote:
>>>
>>>
>>> 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
>>
>> In ldap2.create_connection(), you should set self.time_limit only if
>> time_limit is not None, ditto for size limit, instead of handling None
>> in the properties' setters.
>>
>> You don't need to handle the limits in ldap2.find_entries() anymore,
>> since it is handled by LDAPClient.find_entries(), so you can remove
>> the related code completely from ldap2.find_entries().
>>
> Updated patch attached.

The defaults should be set in LDAPClient class attributes rather than in 
__init__().

Besides that ACK.

To speed things up I have amended your patch myself, see attachment.

Pushed to master: 2a1a3c498a71e85193af76a25333ebe9011e6b2a

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0350.5-Upgrade-increase-time-limit-for-upgrades.patch
Type: text/x-patch
Size: 7602 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151201/fe4002e1/attachment.bin>