[Freeipa-devel] [PATCH][RFC] 7 automember rebuild nowait feature added

[Misnyovszki Adam]

On Fri, 21 Mar 2014 10:33:00 +0100
Petr Viktorin <pviktori at redhat.com> wrote:

> On 03/21/2014 10:29 AM, Petr Viktorin wrote:
> > On 03/20/2014 04:22 PM, Misnyovszki Adam wrote:
> >> On Thu, 20 Mar 2014 14:19:51 +0100
> >> Misnyovszki Adam <amisnyov at redhat.com> wrote:
> >>
> >>> On Fri, 14 Mar 2014 13:26:15 -0400
> >>> Rob Crittenden <rcritten at redhat.com> wrote:
> >>>
> >>>> Misnyovszki Adam wrote:
> >>>>> Hi,
> >>>>>
> >>>>> automember-rebuild uses asynchronous 389 task, and returned
> >>>>> success even if the task didn't run. This patch fixes this issue
> >>>>> adding a --nowait parameter to 'ipa automember-rebuild',
> >>>>> defaulting to False, thus when the script runs without it, it
> >>>>> waits for the 'nstaskexitcode' attribute, which means the task
> >>>>> has finished, according to
> >>>>> http://directory.fedoraproject.org/wiki/Task_Invocation_Via_LDAP#Implementation.
> >>>>>
> >>>>> Old usage can be enabled using --nowait.
> >>>>>
> >>>>> https://fedorahosted.org/freeipa/ticket/4239
> >>>>>
> >>>>> Request for comments:
> >>>>> - Should I add a parameter to specify the polling time? (now
> >>>>> 1ms)
> >>>>> - Should I add a parameter to specify the maximum polling
> >>>>> number? Now if something fails about creating the task, it
> >>>>> polls forever.
> >>>>> - Obviously, if these parameters should be added, there should
> >>>>> be a reasonable default for them (~ Required=False, Default=X).
> >>>>
> >>>> I don't think you need a polling time, esp since this is hidden
> >>>> from the user, but I think that is probably too short and you
> >>>> may end up hammering the LDAP server.
> >>>>
> >>>> I also wonder if there should be some maximum wait time. I don't
> >>>> like loops that can never exit. I'm at a loss for what that time
> >>>> should be though. And we'd need to spell out that we gave up
> >>>> waiting, not that the task necessarily failed. So rather than
> >>>> having a polling time option, rename nowait into wait_for=20, so
> >>>> wait for 20 seconds. Or something like that.
> >>>>
> >>>> I'd suggest using get_entry since you already know the full DN
> >>>> and there is only ever one. It would make this much more
> >>>> readable.
> >>>>
> >>>> I wonder if some top-level documentation should be added to flesh
> >>>> this out some more. This does, for example, return False in one
> >>>> case. The meaning for that should be spelled out.
> >>>>
> >>>> rob
> >>>
> >>> Hi,
> >>> personally I would keep --no-wait, with a delay of 1 seconds, and
> >>> a maximum waiting time for 60 seconds. Questions is, do we need to
> >>> parameterize with other parameters(wait-for to the maximum time,
> >>> and/or poll-delay for the delay time, both not required), or it is
> >>> not a case which worth to develop?
> >>> Greets
> >>> Adam
> >>
> >> Hi,
> >> here are the corrections Petr asked, also the other patch conatins
> >> the plugin registration refactor.
> >
> >
> > Thanks!
> >
> > You forgot an alternate summary for the --no-wait case. (Make sure
> > to output the DN in this case, so it's possible to poll for it.)
> >
> >
> >
> > Instead of `task['nstaskexitcode'][0]` please use
> > `task.single_value['nstaskexitcode']`.
> >
> > Here:
> >
> >     raise errors.DatabaseError(
> >         desc=_("Automember rebuild membership task failed"),
> >         info=_("nstaskexitcode = '%s'" %
> > str(task['nstaskexitcode'][0])))
> >
> > there's no need to call str() on %'s argument.
> > Also, use natural language (like "Task exit code: %s"), otherwise
> > there's no need to mark the message for translation.
> >
> >
> 
> And one more thing: Please bump the minor version in the VERSION file 
> when API.txt changes.
> 
> 

Hi,
everything is corrected!
Greets
Adam

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-amisnyov-0007-3-automember-rebuild-nowait-feature-added.patch
Type: text/x-patch
Size: 5118 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140321/eb039a96/attachment.bin>