> 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?
Hi Ivan, please ignore the comment#4: as I've seen the usage after applying
BOOKKEEPER-336 :-)
- Rakesh
-----------------------------------------------------------
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
>
>
