db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dag.Wan...@Sun.COM (Dag H. Wanvik)
Subject Re: [jira] Updated: (DERBY-2926) Replication: Add a log buffer for log records that should be shipped to the slave
Date Fri, 27 Jul 2007 16:54:43 GMT
"Jørgen Løland (JIRA)" <jira@apache.org> 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

Mime
View raw message