[Freeipa-devel] [PATCHES] 143-147 Improve performance with large groups

[Petr Viktorin]

On 07/23/2013 04:54 PM, Petr Viktorin wrote:
> On 07/23/2013 01:30 PM, Martin Kosek wrote:
>> On 07/19/2013 01:10 PM, Petr Vobornik wrote:
>>> On 07/18/2013 05:29 PM, Jan Cholasta wrote:
>>>> On 18.7.2013 17:26, Martin Kosek wrote:
>>>>> On 07/18/2013 05:22 PM, Jan Cholasta wrote:
>>>>>> On 18.7.2013 17:07, Martin Kosek wrote:
>>>>>>> On 07/18/2013 04:53 PM, Jan Cholasta wrote:
>>>>>>>> Added patch which adds new hidden option no_members to suppress
>>>>>>>> membership
>>>>>>>> processing for commands of all objects that have member attributes.
>>>>>>>> This can be
>>>>>>>> used by the WebUI to prevent member lookups where they are not
>>>>>>>> necessary (as we
>>>>>>>> discussed off-line with Martin and Petr).
>>>>>>>>
>>>>>>>> Honza
>>>>>>>>
>>>>>>>
>>>>>>> 1) Should the new option really have "exclude='webui'? I thought
>>>>>>> that Web UI is
>>>>>>> the main and only consumer of this option.
>>>>>>
>>>>>> The 'webui' context doesn't actually exist and the only meaning of
>>>>>> this stanza
>>>>>> is that the option is not shown in the output of the show_mappings
>>>>>> command.
>>>>>>
>>>>>>>
>>>>>>> 2) I would clearly state this is an internal option only, e.g.
>>>>>>>
>>>>>>> + doc=_('INTERNAL: suppress processing of membership attributes.'),
>>>>>>
>>>>>> No other internal option has this kind of thing in its doc and nobody
>>>>>> will see
>>>>>> it anyway, so we might just leave it like that IMHO.
>>>>>
>>>>> OK.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 3) It would be nice to state that this option is mutually exclusive
>>>>>>> with --all,
>>>>>>> but given it is internal anyway and there is no central place to
>>>>>>> define it, we
>>>>>>> do not need to do that.
>>>>>>
>>>>>> The options are not really mutually exclusive at this point, they
>>>>>> can be
>>>>>> specified together, --all takes precedence.
>>>>>
>>>>> Well, they can be specified together, but the option is then NOOP
>>>>> which could
>>>>> confuse users which may have different expectations. Being explicit
>>>>> helps.
>>>>
>>>> I agree.
>>>>
>>>>> But
>>>>> as I said, in this case this is not a requirement.
>>>>
>>>> I agree as well :-)
>>>>
>>>> Honza
>>>>
>>>
>>> Functional ACK for Honza's patch (didn't break Web UI test suite)
>>>
>>> Attaching Web UI patch.
>>>
>>> It:
>>> 1) Removed --all from _find and _show commands used by search pages. All
>>> displayed attributes should be already included in default attributes.
>>>
>>> 2) Removed search_all_attributes - Not needed since introduction of
>>> paging.
>>>
>>> 3) Added --no-members options to search _show commmands.
>>
>> ACK. Pushed both Petr's and Honza's patch to master and ipa-3-2.
>>
>> Martin
>
> These patches sometimes add the attribute "no_members" to groups, which
> results in 7 tests being broken like this:
>
> ======================================================================
> FAIL: test_cli.TestCLIParsing.test_group_add
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>    File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in
> runTest
>      self.test(*self.arg)
>    File
> "/var/lib/jenkins/workspace/freeipa-unittests-f19-devel/ipatests/test_cmdline/test_cli.py",
> line 73, in test_group_add
>      version=API_VERSION)
>    File
> "/var/lib/jenkins/workspace/freeipa-unittests-f19-devel/ipatests/test_cmdline/test_cli.py",
> line 24, in check_command
>      util.assert_deepequal(kw_expected, kw_got)
>    File
> "/var/lib/jenkins/workspace/freeipa-unittests-f19-devel/ipatests/util.py",
> line 338, in assert_deepequal
>      doc, sorted(missing), sorted(extra), expected, got, stack
> AssertionError: assert_deepequal: dict keys mismatch.
>
>    missing keys = []
>    extra keys = ['no_members']
>    expected = {'all': False, 'cn': u'tgroup1', 'raw': False, 'version':
> u'2.62', 'external': False, 'nonposix': False, 'description': u'Test
> group'}
>    got = {'all': False, 'description': u'Test group', 'no_members':
> False, 'raw': False, 'version': u'2.62', 'external': False, 'nonposix':
> False, 'cn': u'tgroup1'}
>    path = ()
>

Correction: they don't add the attribute to output. It's just that the 
CLI tests need to be updated with the new option.

-- 
Petr³