You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Ivan Kelly <iv...@apache.org> on 2012/10/25 17:50:28 UTC
Review Request: BOOKKEEPER-444 Refactor pending read op to make speculative
reads possible
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7733/
-----------------------------------------------------------
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
Re: Review Request: BOOKKEEPER-444 Refactor pending read op to make
speculative reads possible
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7733/
-----------------------------------------------------------
(Updated Oct. 31, 2012, 3:32 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 (updated)
-----
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
Re: Review Request: BOOKKEEPER-444 Refactor pending read op to make
speculative reads possible
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7733/#review13025
-----------------------------------------------------------
Ship it!
looks good to me. +1 thanks Ivan.
- Sijie Guo
On Oct. 31, 2012, 3:32 p.m., Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7733/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2012, 3:32 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
>
>
Re: Review Request: BOOKKEEPER-444 Refactor pending read op to make
speculative reads possible
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7733/
-----------------------------------------------------------
(Updated Oct. 31, 2012, 3:32 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
Re: Review Request: BOOKKEEPER-444 Refactor pending read op to make
speculative reads possible
Posted by Rakesh R <ra...@huawei.com>.
> 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
>
>
Re: Review Request: BOOKKEEPER-444 Refactor pending read op to make
speculative reads possible
Posted by Ivan Kelly <iv...@apache.org>.
> 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
>
>
Re: Review Request: BOOKKEEPER-444 Refactor pending read op to make
speculative reads possible
Posted by Rakesh R <ra...@huawei.com>.
> 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?
> >
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
-----------------------------------------------------------
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
>
>
Re: Review Request: BOOKKEEPER-444 Refactor pending read op to make
speculative reads possible
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7733/#review12863
-----------------------------------------------------------
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
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
>
>
Re: Review Request: BOOKKEEPER-444 Refactor pending read op to make
speculative reads possible
Posted by Ivan Kelly <iv...@apache.org>.
> On Oct. 29, 2012, 10:38 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java, line 92
> > <https://reviews.apache.org/r/7733/diff/1/?file=179878#file179878line92>
> >
> > do we need to release semaphore when interrupted, since sendReadTo would acquire semaphore.
InterruptedException will only occur when trying to acquire the semaphore. If it does, it's safe to assume we didnt acquire it.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7733/#review12869
-----------------------------------------------------------
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
>
>
Re: Review Request: BOOKKEEPER-444 Refactor pending read op to make
speculative reads possible
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7733/#review12869
-----------------------------------------------------------
looks good except one minor comment.
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java
<https://reviews.apache.org/r/7733/#comment27668>
do we need to release semaphore when interrupted, since sendReadTo would acquire semaphore.
- Sijie Guo
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
>
>