[Freeipa-devel] [RANT] Patchwork process

[Dmitri Pal]

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.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/