I had a look at your patch (v1-fixheader). Please see my comments
1) I think it would be good if there were some class-level javadoc
comments explaining the purpose of each class. For instance, it is not
clear to me what a class named "LogBufferReplication" is meant to do
(I noticed that you mentioned a class called ReplicationLogBuffer in a
JIRA comment. If that's the same class, it sounds like a better name
to me.) Also, javadoc comments for the methods (at least the public
ones) would be good.
2) I got this warning when I built the javadoc:
[javadoc] /export/home/kh160127/derby/trunk/java/engine/org/apache/derby/impl/services/replication/buffer/LogBufferReplication.java:103: warning - @param argument "data_offset" is not a parameter name.
3) If the value of a field is not meant to change during the lifetime
of an object, I find it very useful to mark them as such by declaring
them final. Declaring them final serves both as documentation and as
an extra compile-time error check (and I have heard people saying it
helps the garbage collector as well). These fields could be final:
4) The fields LogBufferReplication.outBufferCapacity and
LogBufferElement.bufferSize are redundant since they are always equal
to outBufferData.length and bufferdata.length.
5) Is the LogBufferElement class supposed to be accessed directly by
classes in other packages? If not, you could remove the public
modifier in the class definition.
6) I think a better name for LogBufferElement.writeByte() would be
writeBytes(), since it writes an array, not a single byte.
7) LogBufferElement.writeInt() and LogBufferElement.writeLong()
perform unnecessary casting and masking of intermediate results. I
think writeLong() could be simplified to:
bufferdata[p++] = (byte) (l >> 56);
bufferdata[p++] = (byte) (l >> 8);
bufferdata[p++] = (byte) l;
As a side note, if this code can use java.nio.ByteBuffer (it can't if
it's supposed to run under J2ME), I would recommend switching to it as
it has helper methods which do exactly the same. The number of Derby
classes with their own methods for big-endian encoding of integers is
high enough already...
8) LogBufferReplication.next() has an empty catch block. What about
9) I think I would have renamed setRecycle() and recycle() to
setRecyclable() and isRecyclable() (when I saw the call to recycle() I
first thought it was a command, not a getter method). Perhaps the
setRecycle() method even could be removed and instead we could pass
the value directly to the constructor?
10) LogBufferReplication.validData() is declared to throw
NoSuchElementException, but it never throws anything.
11) LogBufferReplication.getData() could be simplified by using
12) You asked whether it was OK with internal exceptions like
LogBufferFullException. I think is, but if the exception is supposed
to be caught by other modules (I don't know if that's how it will be
used) perhaps it should be located in one of the iapi packages instead
13) I noticed some use of synchronization in
LogBufferReplication. Could you add a short comment in the class
javadoc stating the synchronization requirements?
14) In LogBufferReplication.next(), could we skip the shrinking of
outBufferData? Unless the buffer can become very large, I think we
could just skip it. That would simplify the code and also reduce the
need for reallocation if there's a non-default buffer size at a later
point in time.