[SSSD] [Freeipa-devel] [RANT] Patchwork process

Simo Sorce simo at redhat.com
Fri Nov 2 14:56:17 UTC 2012


On Fri, 2012-11-02 at 09:16 -0400, Dmitri Pal wrote:
> On 11/02/2012 07:22 AM, Petr Spacek wrote:
> > On 11/02/2012 11:10 AM, Petr Viktorin wrote:
> >> On 11/02/2012 10:46 AM, Martin Kosek wrote:
> >>> On 11/01/2012 07:28 PM, Simo Sorce wrote:
> >>>> On Thu, 2012-11-01 at 10:59 -0400, Rob Crittenden wrote:
> >>>>> Rob Crittenden wrote:
> >>>>>> Simo Sorce wrote:
> >>>>>>> From: Simo Sorce <ssorce at redhat.com>
> >>>>>>>
> >>>>>>> We check (possibly different) data from LDAP only at (re)start.
> >>>>>>> This way we always shutdown exactly the services we started even if
> >>>>>>> the list
> >>>>>>> changed in the meanwhile (we avoid leaving a service running
> >>>>>>> even if
> >>>>>>> it was
> >>>>>>> removed from LDAP as the admin decided it should not be started in
> >>>>>>> future).
> >>>>>>>
> >>>>>>> This should also fix a problematic deadlock with systemd when we
> >>>>>>> try
> >>>>>>> to read
> >>>>>>> the list of service from LDAP at shutdown.
> >>>>>>
> >>>>>> This fixes things for me but ipactl start isn't working reliably and
> >>>>>> I've yet to determine if it is related or not (I'm thinking not).
> >>>>>>
> >>>>>> What I see is that it considers pki-tomcatd to not have started.
> >>>>>> What is
> >>>>>> happening is the request to the getStatus URI is timing out and that
> >>>>>> exception is being considered a "didn't start."
> >>>>>>
> >>>>>> I modified the code to treat a timeout as a 503 and it is still
> >>>>>> failing
> >>>>>> because the timeout is so longer that it eats up all our normal
> >>>>>> timeout
> >>>>>> for waiting for the service at all.
> >>>>>>
> >>>>>> I don't see a way to pass a timeout to the http request method, I'll
> >>>>>> keep looking.
> >>>>>>
> >>>>>> I'm testing in F-18 btw.
> >>>>>
> >>>>> I can't reproduce the startup issues this morning, I still don't
> >>>>> think
> >>>>> they are related to this patch, so ACK.
> >>>>>
> >>>>> pushed all 3 to master and ipa-3-0
> >>>>
> >>>> You pushed an older revision for patch 2, I reverted it on both trees
> >>>> and pushed the right one.
> >>>>
> >>>> Simo.
> >>>
> >>> While trying to follow this thread, I must throw my 2 conservative
> >>> cents in.
> >>> This thread is a very good example why processing our patches via
> >>> patchwork is
> >>> IMHO rather fuzzy and can cause more confusion than clarity. These
> >>> are my main
> >>> points:
> >>>
> >>> I cannot see which patches are the recent ones. Each patch is sent
> >>> separately
> >>> in mail body and I cannot quickly see by looking in thread which
> >>> mail contains
> >>> a revised patch and which is just a comment.
> >>>
> >>> When patches are attached directly to mail as files, I can quickly
> >>> distinguish
> >>> which mail has new patch set as it has "the paperclip icon". It also
> >>> enabled us
> >>> to attach a whole set of valid patches to one mail and thus make it
> >>> easier for
> >>> reviewer to pull a whole set of valid patches in one click.
> >>  >
> >>>
> >>> Patchwork style patch sending also generates a lot of mails which
> >>> may confuse
> >>> not only me but other contributors...
> >>
> >> +1
> >>
> >> This particular thread is extremely confusing. Each individual patch
> >> generated
> >> its own subthread, and then the revised patches were sent as a reply the
> >> original, spawning new threads to continue the old ones. The
> >> discussion is
> >> broken up and impossible to navigate. You can't see where the patches
> >> are, let
> >> alone where to look for latest versions.
> >> Thanks to Rob for showing us that even a person following the thread
> >> is easily
> >> confused.
> >>
> >> Patchwork adds a view that is incomplete and not cross-linked.
> >> To make a comment on a patch, you need to do it by e-mail; finding a
> >> patch in
> >> Patchwork is useless for this. Same for assigning a reviewer: you
> >> need to find
> >> it "manually" in Patchwork even if you have the mail in front of you.
> >>
> >>
> >> I don't really see Patchwork's advantage. For a list of "active"
> >> patches, we
> >> have the "Patch submitted" bit in Trac; to claim a larger review we
> >> could have
> >> a policy to announce this on the list. And the claim that Patchwork
> >> doesn't
> >> change our current practices is simply false.
> >>
> >
> > As guys described above, Patchwork influenced our process a lot and,
> > personally, I don't like these changes.
> >
> > +1 from me to more rational Trac usage. We can simply set bit and/or
> > reviewer name in Trac through API and avoid all changes introduced
> > with Patchwork.
> >
> > Simple script and appropriate Trac query can do better work for us.
> >
> OK. It looks like it brings more disruption than benefit.
> Simo, can we please get back to the original method at least for now?
> At this stage of development it see a lot of disruption that we can't
> afford.
> Patch tracking problem exists - I do not deny it but it seems that the
> change to patchwork does not bring the benefits you hoped.
> I have an action item to explore what we can do with other tools we
> considered like Gerrit. We might be not that far from it becoming an
> option. I will follow up and let you know.

Personally after a lot of consideration I really do not like the Gerrit
option.

It seem that patchwork is not working as is, I wonder if some work on it
to allow it to be able to track multiple patches per email would be
enough to keep using it while not disrupting the traditional way  ?

I have to be honest, I do like the change patchwork introduced, it makes
it a lot easier to me to review patches inline and allows me to easily
download them in a separate machine with a simple wget.

Maybe there is a way to compromise on the rough edges and please
everyone ?

I do not like the trac approach because it is not automatic, so it is
completely inconsistent, and also because trac is extremely slow.
The patchwork webui is fast, I get results alsmot instantaneously, my
experience with trac is that it take several seconds for the simplest
views. Also trac cannot 'trac' patches that are not associated to a bug,
so it will always be incomplete.

The big win I see in patchwork is the automation, which is not in full
force yet and can be improved.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list