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

[Jan Cholasta]

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().

-- 
Jan Cholasta