[Freeipa-devel] [RANT] Patchwork process

[Jan Cholasta]

On 2.11.2012 15:56, Simo Sorce wrote:
> 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.

In Trac's defense, it is actually fedorahosted that is slow. See how 
fast it is on other sites, e.g. <https://dev.openwrt.org> or 
<http://bind10.isc.org>.

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

Honza

-- 
Jan Cholasta