You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Sijie Guo <gu...@gmail.com> on 2012/04/16 15:57:54 UTC
Review Request: BOOKKEEPER-218: Provide journal manager to manage journal
related operations
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4737/
-----------------------------------------------------------
Review request for bookkeeper.
Summary
-------
Currently we put all journal related operations in Bookie class. It would be better to provide a journal manager to provide journal related operations. It would make Bookie logic more clearly.
Besides that, some admin tools like BOOKKEEPER-183 needs to provide could use JournalManager to read/check journal files directly.
This addresses bug BOOKKEEPER-218.
https://issues.apache.org/jira/browse/BOOKKEEPER-218
Diffs
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java bbf561a
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieBean.java fa5171b
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieMXBean.java 8418469
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java c172224
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieJournalRollingTest.java e795fce
Diff: https://reviews.apache.org/r/4737/diff
Testing
-------
Thanks,
Sijie
Re: Review Request: BOOKKEEPER-218: Provide journal manager to manage
journal related operations
Posted by Sijie Guo <gu...@gmail.com>.
> On 2012-04-16 14:50:22, Ivan Kelly wrote:
> >
Thanks Ivan for reviewing. would like to update them in new patch.
only one place about LastLogMark class I would like to explain. I would like to provide a tool to read journal file in BOOKKEEPER-183, including printing current LastLogMark. so admin guys could know the status of journal.
so I leave LastLogMark as package accessible. does it make sense for you?
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4737/#review6935
-----------------------------------------------------------
On 2012-04-16 13:57:54, Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4737/
> -----------------------------------------------------------
>
> (Updated 2012-04-16 13:57:54)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> Currently we put all journal related operations in Bookie class. It would be better to provide a journal manager to provide journal related operations. It would make Bookie logic more clearly.
>
> Besides that, some admin tools like BOOKKEEPER-183 needs to provide could use JournalManager to read/check journal files directly.
>
>
> This addresses bug BOOKKEEPER-218.
> https://issues.apache.org/jira/browse/BOOKKEEPER-218
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java bbf561a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieBean.java fa5171b
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieMXBean.java 8418469
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java c172224
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieJournalRollingTest.java e795fce
>
> Diff: https://reviews.apache.org/r/4737/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie
>
>
Re: Review Request: BOOKKEEPER-218: Provide journal manager to manage
journal related operations
Posted by Ivan Kelly <iv...@apache.org>.
> On 2012-04-16 14:50:22, Ivan Kelly wrote:
> >
>
> Sijie Guo wrote:
> Thanks Ivan for reviewing. would like to update them in new patch.
>
> only one place about LastLogMark class I would like to explain. I would like to provide a tool to read journal file in BOOKKEEPER-183, including printing current LastLogMark. so admin guys could know the status of journal.
>
> so I leave LastLogMark as package accessible. does it make sense for you?
re: LastLogMark, thats fine. I simply didn't see why it needed to be accessible in the current code base, but if you have a plan for it there's no issue.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4737/#review6935
-----------------------------------------------------------
On 2012-04-17 05:50:11, Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4737/
> -----------------------------------------------------------
>
> (Updated 2012-04-17 05:50:11)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> Currently we put all journal related operations in Bookie class. It would be better to provide a journal manager to provide journal related operations. It would make Bookie logic more clearly.
>
> Besides that, some admin tools like BOOKKEEPER-183 needs to provide could use JournalManager to read/check journal files directly.
>
>
> This addresses bug BOOKKEEPER-218.
> https://issues.apache.org/jira/browse/BOOKKEEPER-218
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 00cba69
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieBean.java fa5171b
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieMXBean.java 8418469
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java c172224
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieJournalRollingTest.java e795fce
>
> Diff: https://reviews.apache.org/r/4737/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie
>
>
Re: Review Request: BOOKKEEPER-218: Provide journal manager to manage
journal related operations
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4737/#review6935
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15468>
I'd prefer if this was simply called Journal. JournalManager is a class in HDFS, so having it here also will muddle things in my mind.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15467>
Should be private
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15466>
Should be private
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15469>
private
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15470>
private
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15471>
private
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15472>
I would suggest calling the thread BookieJournal- so that it can be be identified when running a lot of other systems in a single process, such as for testing.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15473>
never used, remove
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15474>
It would be good to have a comment here stating what marking the log means.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15475>
private
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15476>
I would suggest calling this simply replay()
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java
<https://reviews.apache.org/r/4737/#comment15477>
There's an asymmetry between this and the method for reading, JournalScanner.
This takes an entry, a ledgerId, and an entryId.
JournalScanner only gets an entry buffer back.
Logically Journal shouldn't care about entryid or ledgerid. I think logAddEntry should only have (ByteBuffer entry, WriteCallback cb, Object ctx) as the parameter list.
- Ivan
On 2012-04-16 13:57:54, Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4737/
> -----------------------------------------------------------
>
> (Updated 2012-04-16 13:57:54)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> Currently we put all journal related operations in Bookie class. It would be better to provide a journal manager to provide journal related operations. It would make Bookie logic more clearly.
>
> Besides that, some admin tools like BOOKKEEPER-183 needs to provide could use JournalManager to read/check journal files directly.
>
>
> This addresses bug BOOKKEEPER-218.
> https://issues.apache.org/jira/browse/BOOKKEEPER-218
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java bbf561a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieBean.java fa5171b
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieMXBean.java 8418469
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalManager.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java c172224
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieJournalRollingTest.java e795fce
>
> Diff: https://reviews.apache.org/r/4737/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie
>
>
Re: Review Request: BOOKKEEPER-218: Provide journal manager to manage
journal related operations
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4737/#review6973
-----------------------------------------------------------
Ship it!
lgtm +1
- Ivan
On 2012-04-17 05:50:11, Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4737/
> -----------------------------------------------------------
>
> (Updated 2012-04-17 05:50:11)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> Currently we put all journal related operations in Bookie class. It would be better to provide a journal manager to provide journal related operations. It would make Bookie logic more clearly.
>
> Besides that, some admin tools like BOOKKEEPER-183 needs to provide could use JournalManager to read/check journal files directly.
>
>
> This addresses bug BOOKKEEPER-218.
> https://issues.apache.org/jira/browse/BOOKKEEPER-218
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 00cba69
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieBean.java fa5171b
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieMXBean.java 8418469
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java c172224
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieJournalRollingTest.java e795fce
>
> Diff: https://reviews.apache.org/r/4737/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie
>
>
Re: Review Request: BOOKKEEPER-218: Provide journal manager to manage
journal related operations
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4737/#review6974
-----------------------------------------------------------
Ship it!
lgtm +1
- Ivan
On 2012-04-17 10:56:27, Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4737/
> -----------------------------------------------------------
>
> (Updated 2012-04-17 10:56:27)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> Currently we put all journal related operations in Bookie class. It would be better to provide a journal manager to provide journal related operations. It would make Bookie logic more clearly.
>
> Besides that, some admin tools like BOOKKEEPER-183 needs to provide could use JournalManager to read/check journal files directly.
>
>
> This addresses bug BOOKKEEPER-218.
> https://issues.apache.org/jira/browse/BOOKKEEPER-218
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 128fc09
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieBean.java fa5171b
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieMXBean.java 8418469
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java c172224
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieJournalRollingTest.java e795fce
>
> Diff: https://reviews.apache.org/r/4737/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie
>
>
Re: Review Request: BOOKKEEPER-218: Provide journal manager to manage
journal related operations
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4737/
-----------------------------------------------------------
(Updated 2012-04-17 10:56:27.709132)
Review request for bookkeeper.
Changes
-------
rebase the patch to latest trunk.
Summary
-------
Currently we put all journal related operations in Bookie class. It would be better to provide a journal manager to provide journal related operations. It would make Bookie logic more clearly.
Besides that, some admin tools like BOOKKEEPER-183 needs to provide could use JournalManager to read/check journal files directly.
This addresses bug BOOKKEEPER-218.
https://issues.apache.org/jira/browse/BOOKKEEPER-218
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 128fc09
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieBean.java fa5171b
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieMXBean.java 8418469
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java c172224
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieJournalRollingTest.java e795fce
Diff: https://reviews.apache.org/r/4737/diff
Testing
-------
Thanks,
Sijie
Re: Review Request: BOOKKEEPER-218: Provide journal manager to manage
journal related operations
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4737/
-----------------------------------------------------------
(Updated 2012-04-17 05:50:11.011776)
Review request for bookkeeper.
Changes
-------
attach a new patch addressed Ivan's comments.
Summary
-------
Currently we put all journal related operations in Bookie class. It would be better to provide a journal manager to provide journal related operations. It would make Bookie logic more clearly.
Besides that, some admin tools like BOOKKEEPER-183 needs to provide could use JournalManager to read/check journal files directly.
This addresses bug BOOKKEEPER-218.
https://issues.apache.org/jira/browse/BOOKKEEPER-218
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 00cba69
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieBean.java fa5171b
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieMXBean.java 8418469
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java c172224
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieJournalRollingTest.java e795fce
Diff: https://reviews.apache.org/r/4737/diff
Testing
-------
Thanks,
Sijie