Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Zookeeper >> mail # dev >> cleanup and subjective patches


Copy link to this message
-
Re: cleanup and subjective patches
those pieces of code are core to the system. as i said for
reasonsearlier, risking destabilizing them to make the code look
prettier forsome definition of pretty does not seem wise to me.
changing code toadd functionality such as testability, performance,
bug fix, etc doesseem wise.
even then, i tend to be conservative. when i did the protocol fix,
ireally wanted to clean up some things, but i knew if i cleaned up
andfixed the protocol at the same time i both risked introducing a
bugand it made it harder to see exactly what i was changing
forreviewers. i figured it would be better to wait to do a cleanup
whenwe put in a real unit test so that when the refactoring happens we
canknow that it is useful.
i'm fine with losing this argument, but if you guys really believethat
i'm wrong please start committing the cleanup patches! i'm notgoing
to, but i'm fine if another committer takes responsibility forit.
either way, there are some rather big patches out there that
arerotting and may conflict with current and future patches so
decisionsneed to be made.
ben
On Mon, Oct 31, 2011 at 6:47 PM, Mahadev Konar <[EMAIL PROTECTED]> wrote:
> I'd have to agree with Pat here. I think we should be open to code
> cleanup especially code cleanup that makes testing easier. There is no
> harm in folks contributing minor cleanups (all the more easier to
> review). It is a nice way of encouraging folks to contribute more and
> get more involved in the community.
>
> I'd personally like some cleanup in the code especially -
>
> 1) NIO  code
> 2) Leader Election code
> 3) QuorumPeer/Leader/Follower/ZooKeeperServer interaction.
>
> This would make testing easier and would make the code maintainable in
> the longer run.
>
> thanks
> mahadev
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <[EMAIL PROTECTED]> wrote:
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <[EMAIL PROTECTED]> wrote:
>>> Jumping over to Ben's side of the discussion,
>>>
>>> Git helps with this, but does not eliminate the problem.  At some point the
>>> changes become difficult to understand relative to the new code and may
>>> even collide in ways that are difficult to merge.
>>>
>>> It is true, however, that patches can be kept live using tools like git.
>>>  That is how I kept the multi stuff alive, but there was at least one
>>> tricky merge to be done.
>>>
>>> On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <[EMAIL PROTECTED]> wrote:
>>>
>>>> Thanks to Ted for replying. I will save the mail I started in the drafts
>>>> folder until I've calmed down again.