incubator-cassandra-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jonathan Ellis <jbel...@gmail.com>
Subject Re: working together
Date Wed, 08 Apr 2009 04:02:22 GMT
On Tue, Apr 7, 2009 at 10:11 PM, Avinash Lakshman
<avinash.lakshman@gmail.com> wrote:
> The part that is very disconcerting are the following:
> (1) If one becomes a committer one is not expected to blitz through the code
> base and start refactoring everything.

There are two reasons I refactor.

One is, I always try to leave the code better than I found it.  This
helps fight technical debt.  In a project the size of cassandra you
have to do this or you end up with fragile code that you cannot change
without introducing regressions.  Usually this is not because the
original code was bad, but because the requirements changed as the
project grew and now there is a better way.  Or sometimes there is an
idiom better suited to a situation that the original author was not
familiar with.  That's okay too; nobody's perfect.

An example of this is r762440, replace String.indexOf != -1 with
String.contains (review by johano).

Two is, if I am going to introduce a new feature I will try to
refactor first without changing behavior in such away that the feature
becomes easier to add.  This breaks the changes up into smaller pieces
which are easier to review and easier to validate against regressions.
 (The more things you change at once, the harder it is to find which
caused a problem.)

A specific class of this kind of change is merging copy/pasted code
into method calls.  Duplicate code makes it hard to add features
because you have to know about the duplicates and remember change all
when you change one.  There is a lot of this in cassandra.  Examples
include r762440, r762440, r762440.  (This specific area of the code
did bite me while implementing remove; that's what the "os x error"
thread on google groups was about.)

So there is a method to my madness. :)

> In any organization one doesn't just go about ripping out everyone
> else's code for no rhyme or reason. That will offend anybody. I personally
> would not go about ripping someone else's code apart if I had become
> committer. It is just that respect ought to be there.

I think there may be cultural differences here.  It is not a sign of
disrespect to think that I can improve your code or anyone's. :)

> (2) This is something that I have said many times over. Certain things are
> the way they are for a reason.

And others are not.  For instance, there's a lot of classes that still
implement Serializable, but that's misleading because you roll your
own serialization now.

Or for something more meaningful, the code-by-copy-paste examples.

It's just not reasonable to say "don't touch anything, it's all done
for a reason" when there are clear counterexamples right there in the
code.

I don't see any alternative to your becoming involved in the review
process so you can tell us when something really is important, such as

> For example when I say ConcurrentHashMap is a
> memory hog I say it because we have seen this in practice. How does it
> manifest itself? I obviously do not recall since all this was over a year
> ago.

So I replaced those with NonBlockingHashMap.  The system is working. :)

> No one can claim to have run tests the way we have in the last year and
> a half. One cannot just run some simple test and say well I do not see the
> problem. I am not dumb.

Again, maybe there are cultural differences here...  Todd was asking
out of curiosity and a desire for more information.  Often knowing how
you arrived at a conclusion is as useful or more so than the
conclusion itself.  (Otherwise you run the risk of encouraging cargo
cult programming.)

> My understanding was that new committers come in and start with some feature
> implement that and then slowly start looking into what more they could do
> going forward.

That's what happened... in December.  I've had a lot of time to look. :)

> It is NOT come in and refactor the hell out of the system
> because you like something to be in a specific way.

I think I addressed this above, but I'm happy to discuss specific
commits in more detail.

-Jonathan

Mime
View raw message