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: Column is immutable ... no, it's not
Date Wed, 08 Jul 2009 01:21:40 GMT
Please give us some credit.  It's obvious that you can modify the
contents of the byte[], but we're not trying to protect against
malicious plugins or... something; we're only concerned with thread
safety.  For that, the existing code is fine; it's not worth
introducing extra copies to a performance-sensitive part of the code
to guard against an exceptionally contrived example.

(Cassandra never touches the byte[] values; they are opaque to it.  It
doesn't make even a little sense for someone to introduce a bug like
that.)

I suggest that your energy would be better spent picking a
feature-oriented issue from JIRA -- or creating your own -- gaining
some understanding of what the code _does_, and work on improving it
from that angle, rather than trying to micro-cleanup in a vacuum.

https://issues.apache.org/jira/browse/CASSANDRA-212 is low hanging
fruit, reasonably self-contained, and will introduce you to several
important parts of the code, for instance.

Don't get me wrong; there's plenty of places Cassandra's code is
suboptimal.  But you will get more understanding of the code, faster,
by taking a feature-oriented approach.

-Jonathan

On Tue, Jul 7, 2009 at 8:06 PM, Edward Ribeiro<edward.ribeiro@gmail.com> wrote:
> Column's javadoc says that it's an immutable class, but look at the
> following constructor:
>
>    Column(String name, byte[] value, long timestamp, boolean isDeleted)
>    {
>        assert name != null;
>        assert value != null;
>        this.name = name;
>        this.value = value;
>        this.timestamp = timestamp;
>        isMarkedForDelete = isDeleted;
>    }
>
> Everything is OK, right? If you think so, then run the following
> method as a (junit) test :
>
>        public void testColumn() {
>                byte[] buf = "hello".getBytes();
>                Column c = new Column("test", buf);
>                assertEquals("hello", new String(c.value()));
>                buf[1] = 'x';
>                assertEquals("hello", new String(c.value()));
>        }
>
>
> Surprise! It fails at the second assertEquals.
>
> Well, the byte array is a reference to an object that is not under
> control of the Column object. If an object, that is part of the state
> of Column, can be changed then Column is not immutable by any means.
>
> If you think the little test method is not reallistic and nobody will
> ever do this then you are assuming that programmers don't create bugs,
> ever. In addition, the javadoc says something that simply is not true.
> Therefore, you have two options: change the documentation or change
> the constructor.
>
> Let's see what needs to be done to turn Column into an 100% immutable class:
>
> First, I create a utility method that just copies the content of the
> byte array to a internal byte array inside Column class. This is what
> we call a defensive copy. The other arguments are immutable (String)
> or primitive types so they don't need defensive copy. But the byte
> array is an achilles' heel lurking around.
>
>    private byte[] copyArray(byte[] src)
>    {
>        byte[] newArray = new byte[src.length];
>        System.arraycopy(src, 0, newArray, 0, newArray.length);
>        return newArray;
>    }
>
> Then, let's make a little change to the constructor (I ommited parts
> of the code):
>
>    Column(String name, byte[] value, long timestamp, boolean isDeleted)
>    {
>        [....]
>        this.value = copyArray(value);
>        [....]
>    }
>
> Run the tests again it will display a beautiful green bar. :) Now,
> Column is a immutable class by excellence. :)
>
> Edward
> PS: Effective Java by Joshua Bloch, 2nd edition, is the book that have
> this and many other tricks.
>

Mime
View raw message