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/01/27 15:29:27 UTC
Review Request: BOOKKEEPER-137 Do not create Ledger index files until
absolutely necessary.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3661/
-----------------------------------------------------------
Review request for bookkeeper.
Summary
-------
This is an optimization to speed up the case where we have many ledgers and are writing to them at random (a benchmark case we currently have). Currently, we create the ledger index file and write the first 1k of it to disk immediately. With a lot of ledgers being randomly written to, this means a lot of random writes on the ledger disk. This fix postpones the creation of the index file and writing of the first 1k until the first flush of the ledger.
This patch includes BOOKKEEPER-136, as they both deal in the same area, and I found it difficult to separate them.
BOOKKEEPER-135 is not required for this patch, and will need modifications after this goes in.
This addresses bug BOOKKEEPER-137.
https://issues.apache.org/jira/browse/BOOKKEEPER-137
Diffs
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java cb3bb26
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java fa713c8
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java d2f959b
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java 728d729
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java beab5e8
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java 5706dd8
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java c7b07e6
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java 7403604
Diff: https://reviews.apache.org/r/3661/diff
Testing
-------
Thanks,
Ivan
Re: Review Request: BOOKKEEPER-137 Do not create Ledger index files until
absolutely necessary.
Posted by Ivan Kelly <iv...@apache.org>.
> On 2012-02-08 15:39:47, Sijie Guo wrote:
> > most is good to me. thanks Ivan, except a BookieException is missing in BaseTestCase that I commented as below.
> >
> > and it seems that new patch doesn't fix the readHeader twice problem.
Ah, will fix the BookieException thing and upload a new patch against trunk.
The readHeader issue has been fixed. It can still be called twice, but if the file has already been opened, it returns immediately.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3661/#review4903
-----------------------------------------------------------
On 2012-02-07 18:13:15, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3661/
> -----------------------------------------------------------
>
> (Updated 2012-02-07 18:13:15)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> This is an optimization to speed up the case where we have many ledgers and are writing to them at random (a benchmark case we currently have). Currently, we create the ledger index file and write the first 1k of it to disk immediately. With a lot of ledgers being randomly written to, this means a lot of random writes on the ledger disk. This fix postpones the creation of the index file and writing of the first 1k until the first flush of the ledger.
>
> This patch includes BOOKKEEPER-136, as they both deal in the same area, and I found it difficult to separate them.
>
> BOOKKEEPER-135 is not required for this patch, and will need modifications after this goes in.
>
>
> This addresses bug BOOKKEEPER-137.
> https://issues.apache.org/jira/browse/BOOKKEEPER-137
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 99b797f
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java fa713c8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java 0fc5206
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java 728d729
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java 2228ab4
> bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java 5706dd8
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java c7b07e6
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java db1a763
>
> Diff: https://reviews.apache.org/r/3661/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan
>
>
Re: Review Request: BOOKKEEPER-137 Do not create Ledger index files until
absolutely necessary.
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3661/#review4903
-----------------------------------------------------------
most is good to me. thanks Ivan, except a BookieException is missing in BaseTestCase that I commented as below.
and it seems that new patch doesn't fix the readHeader twice problem.
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java
<https://reviews.apache.org/r/3661/#comment10799>
this patch applied to trunk would compile fail. due to in BOOKKEEPER-156, start bookie action has been put in startBookie function. so it need to add BookieException also in startBookie.
- Sijie
On 2012-02-07 18:13:15, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3661/
> -----------------------------------------------------------
>
> (Updated 2012-02-07 18:13:15)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> This is an optimization to speed up the case where we have many ledgers and are writing to them at random (a benchmark case we currently have). Currently, we create the ledger index file and write the first 1k of it to disk immediately. With a lot of ledgers being randomly written to, this means a lot of random writes on the ledger disk. This fix postpones the creation of the index file and writing of the first 1k until the first flush of the ledger.
>
> This patch includes BOOKKEEPER-136, as they both deal in the same area, and I found it difficult to separate them.
>
> BOOKKEEPER-135 is not required for this patch, and will need modifications after this goes in.
>
>
> This addresses bug BOOKKEEPER-137.
> https://issues.apache.org/jira/browse/BOOKKEEPER-137
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 99b797f
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java fa713c8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java 0fc5206
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java 728d729
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java 2228ab4
> bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java 5706dd8
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java PRE-CREATION
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java c7b07e6
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java db1a763
>
> Diff: https://reviews.apache.org/r/3661/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan
>
>
Re: Review Request: BOOKKEEPER-137 Do not create Ledger index files until
absolutely necessary.
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3661/#review4966
-----------------------------------------------------------
Ship it!
thanks Ivan for explanation. the new patch is ok for me. +1
- Sijie
On 2012-02-08 15:53:28, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3661/
> -----------------------------------------------------------
>
> (Updated 2012-02-08 15:53:28)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> This is an optimization to speed up the case where we have many ledgers and are writing to them at random (a benchmark case we currently have). Currently, we create the ledger index file and write the first 1k of it to disk immediately. With a lot of ledgers being randomly written to, this means a lot of random writes on the ledger disk. This fix postpones the creation of the index file and writing of the first 1k until the first flush of the ledger.
>
> This patch includes BOOKKEEPER-136, as they both deal in the same area, and I found it difficult to separate them.
>
> BOOKKEEPER-135 is not required for this patch, and will need modifications after this goes in.
>
>
> This addresses bug BOOKKEEPER-137.
> https://issues.apache.org/jira/browse/BOOKKEEPER-137
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 8abe87a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java fa713c8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java 10ecac7
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java 771c0ba
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java 728d729
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java 2228ab4
> bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java 5706dd8
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java ae63710
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java c7b07e6
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java 9750158
>
> Diff: https://reviews.apache.org/r/3661/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan
>
>
Re: Review Request: BOOKKEEPER-137 Do not create Ledger index files until
absolutely necessary.
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3661/
-----------------------------------------------------------
(Updated 2012-02-08 15:53:28.318711)
Review request for bookkeeper.
Changes
-------
Fixed compile error
Summary
-------
This is an optimization to speed up the case where we have many ledgers and are writing to them at random (a benchmark case we currently have). Currently, we create the ledger index file and write the first 1k of it to disk immediately. With a lot of ledgers being randomly written to, this means a lot of random writes on the ledger disk. This fix postpones the creation of the index file and writing of the first 1k until the first flush of the ledger.
This patch includes BOOKKEEPER-136, as they both deal in the same area, and I found it difficult to separate them.
BOOKKEEPER-135 is not required for this patch, and will need modifications after this goes in.
This addresses bug BOOKKEEPER-137.
https://issues.apache.org/jira/browse/BOOKKEEPER-137
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 8abe87a
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java fa713c8
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java 10ecac7
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java 771c0ba
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java 728d729
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java 2228ab4
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java 5706dd8
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java ae63710
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java c7b07e6
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java 9750158
Diff: https://reviews.apache.org/r/3661/diff
Testing
-------
Thanks,
Ivan
Re: Review Request: BOOKKEEPER-137 Do not create Ledger index files until
absolutely necessary.
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3661/
-----------------------------------------------------------
(Updated 2012-02-07 18:13:15.812700)
Review request for bookkeeper.
Changes
-------
New diff, addresses Sijie's comments. Diffed against trunk with BOOKKEEPER-165 applied.
Summary
-------
This is an optimization to speed up the case where we have many ledgers and are writing to them at random (a benchmark case we currently have). Currently, we create the ledger index file and write the first 1k of it to disk immediately. With a lot of ledgers being randomly written to, this means a lot of random writes on the ledger disk. This fix postpones the creation of the index file and writing of the first 1k until the first flush of the ledger.
This patch includes BOOKKEEPER-136, as they both deal in the same area, and I found it difficult to separate them.
BOOKKEEPER-135 is not required for this patch, and will need modifications after this goes in.
This addresses bug BOOKKEEPER-137.
https://issues.apache.org/jira/browse/BOOKKEEPER-137
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 99b797f
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java fa713c8
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java 0fc5206
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java 728d729
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java 2228ab4
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java 5706dd8
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java PRE-CREATION
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java c7b07e6
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java db1a763
Diff: https://reviews.apache.org/r/3661/diff
Testing
-------
Thanks,
Ivan
Re: Review Request: BOOKKEEPER-137 Do not create Ledger index files until
absolutely necessary.
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3661/#review4819
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
<https://reviews.apache.org/r/3661/#comment10582>
suppose the code running in journalLayoutVersion=1 directory. the first time is OK, since there would be no meta entries. but the code would add meta entries then. the next time bookie restarts, journalLayoutVersion is still 1, it would throw IOException.
so do we need to upgrade the layout version when we run new code in old data directories?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
<https://reviews.apache.org/r/3661/#comment10583>
if a ledger existed before, getHandle would checkAccess of masterKey, which would call FileInfo#readHeader. Then isMasterKeyPersisted would call FileInfo#readHeader again, which is not necessary.
I think it would be easy to avoid readHeader twice.
- Sijie
On 2012-01-27 14:54:04, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3661/
> -----------------------------------------------------------
>
> (Updated 2012-01-27 14:54:04)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> This is an optimization to speed up the case where we have many ledgers and are writing to them at random (a benchmark case we currently have). Currently, we create the ledger index file and write the first 1k of it to disk immediately. With a lot of ledgers being randomly written to, this means a lot of random writes on the ledger disk. This fix postpones the creation of the index file and writing of the first 1k until the first flush of the ledger.
>
> This patch includes BOOKKEEPER-136, as they both deal in the same area, and I found it difficult to separate them.
>
> BOOKKEEPER-135 is not required for this patch, and will need modifications after this goes in.
>
>
> This addresses bug BOOKKEEPER-137.
> https://issues.apache.org/jira/browse/BOOKKEEPER-137
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java cb3bb26
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java fa713c8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java d2f959b
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java 728d729
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java beab5e8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java 5706dd8
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java c7b07e6
> bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java 7403604
>
> Diff: https://reviews.apache.org/r/3661/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan
>
>
Re: Review Request: BOOKKEEPER-137 Do not create Ledger index files until
absolutely necessary.
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3661/
-----------------------------------------------------------
(Updated 2012-01-27 14:54:04.113114)
Review request for bookkeeper.
Changes
-------
There was one compile error fix I hadn't committed.
Summary
-------
This is an optimization to speed up the case where we have many ledgers and are writing to them at random (a benchmark case we currently have). Currently, we create the ledger index file and write the first 1k of it to disk immediately. With a lot of ledgers being randomly written to, this means a lot of random writes on the ledger disk. This fix postpones the creation of the index file and writing of the first 1k until the first flush of the ledger.
This patch includes BOOKKEEPER-136, as they both deal in the same area, and I found it difficult to separate them.
BOOKKEEPER-135 is not required for this patch, and will need modifications after this goes in.
This addresses bug BOOKKEEPER-137.
https://issues.apache.org/jira/browse/BOOKKEEPER-137
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java cb3bb26
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java fa713c8
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCache.java d2f959b
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerDescriptor.java 728d729
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java beab5e8
bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java 5706dd8
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieLayoutVersionTest.java c7b07e6
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java 8526db5
bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BaseTestCase.java 7403604
Diff: https://reviews.apache.org/r/3661/diff
Testing
-------
Thanks,
Ivan