> On Oct. 28, 2012, 7:45 a.m., Rakesh R wrote:
> > Hi Ivan, Patch looks nice. Just few improvements.
> >
> >
> >
> > To make it simple, can we move lh.opCounterSem.release() inside
> > submitCallback() method rather than scattering acquire/release pair many
> > places. How does it sound?
> >
>
> Rakesh R wrote:
> Adding few more comments:
> comment#2:
> Also, please remove the logging: LOG.debug("Releasing lock: {}",
> entryId); present inside readEntryComplete() method, it may mislead.
> comment#3:
> There is few unused imports in PendingReadOp.java like: import
> ByteBuffer, BookieProtocol, IOException; Could you please cleanup this also.
> comment#4:
> isComplete() method is unused, is it required now?
>
> Rakesh R wrote:
> Hi Ivan, please ignore the comment#4: as I've seen the usage after
> applying BOOKKEEPER-336 :-)
release/acquire only occur once each now. before sending the request to the
bookie, and immediately after hearing back. This is better than having it in
submitCallback, as submitCallback is for the whole operation.
I'll fix 2 & 3.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7733/#review12863
-----------------------------------------------------------
On Oct. 25, 2012, 3:50 p.m., Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7733/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2012, 3:50 p.m.)
>
>
> Review request for bookkeeper.
>
>
> Description
> -------
>
> The code to handle the state of a single entry read request is scattered all
> over PendingReadOp. Some even leaks into LedgerEntry. This jira is to
> refactor this code into one place to make speculative reads easier to
> implement.
>
>
> This addresses bug BOOKKEEPER-444.
> https://issues.apache.org/jira/browse/BOOKKEEPER-444
>
>
> Diffs
> -----
>
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerEntry.java
> 1f4547e
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java
> c8e814c
>
> Diff: https://reviews.apache.org/r/7733/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan Kelly
>
>
