"Jørgen Løland (JIRA)" <[EMAIL PROTECTED]> writes:
> [
> https://issues.apache.org/jira/browse/DERBY-2926?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
> ]
>
> Jørgen Løland updated DERBY-2926:
> ---------------------------------
>
> Attachment: bytebuffer_v1a.stat
> bytebuffer_v1a.diff
>
> Patch v1a replaces patch v1. It removes two methods that were used
> in testing, but is otherwise equal.
Enclosing my comments here since I can't comment on the JIRA for now.
Code looks correct and does what it is intended to do from what I can,
including synchronization (but see comment below). Great!
I would appreciate a package level Javadoc that explains the mechanism in due
course, though :)
My notes, mostly nits. Feel free to ignore style comments ;) Caveat: I
am off on two week-s vacation, so no I can't reply to any questions
about my comments for now, so do with them what you like ;)
* LogBufferReplication
- Header class name is wrong
(org.apache.derby.impl.store.replication.LogBufferLinkedList)
- style: "// <text" (Add blank for readibility and align them if possible
- LOG_RECORD_FIXED_OVERHEAD_SIZE computation hard coded - can this
be avoided? Or at least checked dynamically once..
- Javadoc for some public members missing, for those that have javadoc,
exceptions tags are partly missing, e.g. for getSize
- outBufferCapacity is redundant: use outBufferData.length ?
- outBufferSize: name can be a bit misleading. This is not the size of
the buffer, but the number of bytes stored in it; maybe
"outBufferStored" ?
- constructor: freeBuffers.addLast(b); addFirst faster/more logical?
Not that it matters.. The one last added is immediately withdrawn,
so why insert it? But having two allocation statements probably
only clutters things up, so ok.
- Comment "//Buffer is full if there are no more free buffers"
reads a bit awkwardly, I propose:
// Buffer is full if there are no more free buffer elements"
- switchDirtyBuffer has redundant synchronized block
- style: switchDirtyBuffer: avoid unbraced single line if's
Also in getData, getSize and getInstant
See e.g.
http://java.sun.com/docs/codeconv/html/CodeConventions.doc6.html#449
- switchDirtyBuffer: Why test for freeBuffers.size() when you can
just catch the exception from removeFirst and rethrow
LogBufferFullException? It is probably better to
*first* append currentDirtyBuffer to dirtyBuffers before you throw
anyway, so it's writing doesn't get delayed..
- //currentDirtyBuffer was just swapped and has not been
//touched since. No need to put it into dirtyBuffers
-> wording suggestion:
// currentDirtyBuffer has already been handed over to
// the dirtyBuffers list, and an empty one is in place, so
// no need to touch currentDirtyBuffer here.
- next: //this should not be possible when dirtyBuffers.size() == 0
Add Sanity check code here maybe.
- getInstant: Since this returns the last instant in the outbuffer,
I suggest calling it getLastInstant
* LogBufferElement
- Can you use java,nio.ByteBuffer to save manual serializing
and position book-keeping code?
- asymmetry: writeInt writes to bufferdata. writeByte writes to the
parameter byte buffer b which is a bit confusing. Name change advisable?
E.g. getBytes? But even for writeInt, there is no writing going on,
just serialization/formatting.
- style: space around minus operator "bufferSize-position"
- Javadoc for public members, btw. would package private be enough
visibility here?
- init doesn't really initialize all members, e.g "recycle" is not
set, but in practice recycle will be true, I guess, if init() is
ever called, that it is is recycled. "init" is now really
"reInitialize". I think I would set all state variables in it to
be future-proof, except "bufferdata", and then call it from the
constructor to avoid redundancy (makes the name right too ;)
* LogBufferFullException
- I see you raised the issue whther this is good Derby style or not,
I don't feel strongly about it. In this case I think it makes the
contract clear and is ok (avoids magic values for returned ints).
Dag
