You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Matteo Merli <mm...@yahoo-inc.com> on 2015/04/10 05:41:55 UTC
Review Request 33061: BOOKKEEPER-841: Bookie should calculate ledgers
map writing a new entry log file
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33061/
-----------------------------------------------------------
Review request for bookkeeper.
Bugs: BOOKKEEPER-841
https://issues.apache.org/jira/browse/BOOKKEEPER-841
Repository: bookkeeper-git
Description
-------
Bookie should calculate ledgers map when writing a new entry log file. so the bookie doesn't need to scan that entry log file again, which it would improve garbage collection efficiency
Diffs
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 0e052b5
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java 488f4bf
Diff: https://reviews.apache.org/r/33061/diff/
Testing
-------
Thanks,
Matteo Merli
Re: Review Request 33061: BOOKKEEPER-841: Bookie should calculate
ledgers map writing a new entry log file
Posted by Matteo Merli <mm...@yahoo-inc.com>.
> On May 19, 2015, 7:33 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java, line 49
> > <https://reviews.apache.org/r/33061/diff/4/?file=930852#file930852line49>
> >
> > why change here? the checkpointSource passed to super would be null
I think this was to fix unit tests (and currently addressed in BOOKKEEPER-854)
- Matteo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33061/#review84274
-----------------------------------------------------------
On April 15, 2015, 5:37 p.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33061/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 5:37 p.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-841
> https://issues.apache.org/jira/browse/BOOKKEEPER-841
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Bookie should calculate ledgers map when writing a new entry log file. so the bookie doesn't need to scan that entry log file again, which it would improve garbage collection efficiency
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 0e052b5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java 488f4bf
>
> Diff: https://reviews.apache.org/r/33061/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>
Re: Review Request 33061: BOOKKEEPER-841: Bookie should calculate
ledgers map writing a new entry log file
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33061/#review84274
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment135471>
rename ledgersMap to ledgersMapBuffer? because ledgersMap makes people thinking it is a map.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment135469>
could you add entry log id in the IOExcpetion's error message?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment135470>
could you add entry log id in the IOExcpetion's error message?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment135472>
ledgersMap.getLong() will throw BufferUnderflowException, which is a runtime exception. it should be handled.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment135473>
ledgersMap.getLong() will throw BufferUnderflowException, which is a runtime exception. it should be handled.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment135474>
add entry log id in error message?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment135475>
add entry log id in error message
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java
<https://reviews.apache.org/r/33061/#comment135477>
why change here? the checkpointSource passed to super would be null
- Sijie Guo
On April 15, 2015, 5:37 p.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33061/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 5:37 p.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-841
> https://issues.apache.org/jira/browse/BOOKKEEPER-841
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Bookie should calculate ledgers map when writing a new entry log file. so the bookie doesn't need to scan that entry log file again, which it would improve garbage collection efficiency
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 0e052b5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java 488f4bf
>
> Diff: https://reviews.apache.org/r/33061/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>
Re: Review Request 33061: BOOKKEEPER-841: Bookie should calculate
ledgers map writing a new entry log file
Posted by Matteo Merli <mm...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33061/
-----------------------------------------------------------
(Updated April 15, 2015, 5:37 p.m.)
Review request for bookkeeper.
Changes
-------
Put back appendLedgersMap() in createNewLog() to have the index ready when GC thread wants it.
Bugs: BOOKKEEPER-841
https://issues.apache.org/jira/browse/BOOKKEEPER-841
Repository: bookkeeper-git
Description
-------
Bookie should calculate ledgers map when writing a new entry log file. so the bookie doesn't need to scan that entry log file again, which it would improve garbage collection efficiency
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 0e052b5
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java 488f4bf
Diff: https://reviews.apache.org/r/33061/diff/
Testing
-------
Thanks,
Matteo Merli
Re: Review Request 33061: BOOKKEEPER-841: Bookie should calculate
ledgers map writing a new entry log file
Posted by Matteo Merli <mm...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33061/
-----------------------------------------------------------
(Updated April 14, 2015, 11:28 p.m.)
Review request for bookkeeper.
Bugs: BOOKKEEPER-841
https://issues.apache.org/jira/browse/BOOKKEEPER-841
Repository: bookkeeper-git
Description
-------
Bookie should calculate ledgers map when writing a new entry log file. so the bookie doesn't need to scan that entry log file again, which it would improve garbage collection efficiency
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 0e052b5
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java 488f4bf
Diff: https://reviews.apache.org/r/33061/diff/
Testing
-------
Thanks,
Matteo Merli
Re: Review Request 33061: BOOKKEEPER-841: Bookie should calculate
ledgers map writing a new entry log file
Posted by Matteo Merli <mm...@yahoo-inc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33061/
-----------------------------------------------------------
(Updated April 14, 2015, 11:19 p.m.)
Review request for bookkeeper.
Changes
-------
Addressed comments
Bugs: BOOKKEEPER-841
https://issues.apache.org/jira/browse/BOOKKEEPER-841
Repository: bookkeeper-git
Description
-------
Bookie should calculate ledgers map when writing a new entry log file. so the bookie doesn't need to scan that entry log file again, which it would improve garbage collection efficiency
Diffs (updated)
-----
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java PRE-CREATION
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 0e052b5
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java 488f4bf
Diff: https://reviews.apache.org/r/33061/diff/
Testing
-------
Thanks,
Matteo Merli
Re: Review Request 33061: BOOKKEEPER-841: Bookie should calculate
ledgers map writing a new entry log file
Posted by Matteo Merli <mm...@yahoo-inc.com>.
> On April 14, 2015, 7:37 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, line 426
> > <https://reviews.apache.org/r/33061/diff/1/?file=922608#file922608line426>
> >
> > could you move #appendLedgerMap() to flushRotatedLogs()? so appendLedgerMap happens only on checkpointing, rather than when rolling log. because rolling log happens in critical path which will impact latency.
>
> Matteo Merli wrote:
> The problem is that on flushRotatedLogs() we have a list of BufferedLogChannel. I'll change it to keep the EntryLogMetadata in the BufferedLogChannel, so we can have multiple outstanding to be flushed.
Actually, the bigger issue is that when an entry log is rotated, the garbage collector will try to read index and since it doesn't find it, will scan the entry log.
I'll change it back again to do appendLedgersMap() in flushRotatedLogs()
- Matteo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33061/#review79997
-----------------------------------------------------------
On April 14, 2015, 11:28 p.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33061/
> -----------------------------------------------------------
>
> (Updated April 14, 2015, 11:28 p.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-841
> https://issues.apache.org/jira/browse/BOOKKEEPER-841
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Bookie should calculate ledgers map when writing a new entry log file. so the bookie doesn't need to scan that entry log file again, which it would improve garbage collection efficiency
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 0e052b5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java 488f4bf
>
> Diff: https://reviews.apache.org/r/33061/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>
Re: Review Request 33061: BOOKKEEPER-841: Bookie should calculate
ledgers map writing a new entry log file
Posted by Sijie Guo <gu...@gmail.com>.
> On April 14, 2015, 7:37 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, line 105
> > <https://reviews.apache.org/r/33061/diff/1/?file=922608#file922608line105>
> >
> > I don't prefer using a int here. so for format upgrading, the old code could still read new version format by ignoring new metadata entry introduced in newer version. which it would be much rollback friendly.
>
> Matteo Merli wrote:
> I don't understand the concern here. Header version is an integer on disk. V0 version has value '0' which correspond with the zeroed entry logger current headers. newer entry logs will have version 1. Old code is anyway ignoring the header version here, so we will be able to rollback.
the point here is for future. for example, when we add new version 'v2' in future. v1 code should be able to read v2 entry log files. the code in the review (v1 code) couldn't be able to read index file of v2 entry log files.
> On April 14, 2015, 7:37 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, line 426
> > <https://reviews.apache.org/r/33061/diff/1/?file=922608#file922608line426>
> >
> > could you move #appendLedgerMap() to flushRotatedLogs()? so appendLedgerMap happens only on checkpointing, rather than when rolling log. because rolling log happens in critical path which will impact latency.
>
> Matteo Merli wrote:
> The problem is that on flushRotatedLogs() we have a list of BufferedLogChannel. I'll change it to keep the EntryLogMetadata in the BufferedLogChannel, so we can have multiple outstanding to be flushed.
>
> Matteo Merli wrote:
> Actually, the bigger issue is that when an entry log is rotated, the garbage collector will try to read index and since it doesn't find it, will scan the entry log.
>
> I'll change it back again to do appendLedgersMap() in flushRotatedLogs()
no. it isn't issue. the gc thread only scans the flushed entry log files.
> On April 14, 2015, 7:37 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, line 972
> > <https://reviews.apache.org/r/33061/diff/1/?file=922608#file922608line972>
> >
> > we shouldn't limit version. for future version upgrade.
>
> Matteo Merli wrote:
> This check is in the extractEntryLogMetadataFromIndex() method called explicitely to read the index. If the Version is below V1, it means we have no index to read from anyway.
> If the HeaderVersion is later increased to V2, V3... This check will still be valid: ledgersMap index has been added in V1, so it will not be present in any prior versions.
see comments above. this is for future extension.
- Sijie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33061/#review79997
-----------------------------------------------------------
On April 15, 2015, 5:37 p.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33061/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 5:37 p.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-841
> https://issues.apache.org/jira/browse/BOOKKEEPER-841
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Bookie should calculate ledgers map when writing a new entry log file. so the bookie doesn't need to scan that entry log file again, which it would improve garbage collection efficiency
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 0e052b5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java 488f4bf
>
> Diff: https://reviews.apache.org/r/33061/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>
Re: Review Request 33061: BOOKKEEPER-841: Bookie should calculate
ledgers map writing a new entry log file
Posted by Matteo Merli <mm...@yahoo-inc.com>.
> On April 14, 2015, 7:37 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, line 105
> > <https://reviews.apache.org/r/33061/diff/1/?file=922608#file922608line105>
> >
> > I don't prefer using a int here. so for format upgrading, the old code could still read new version format by ignoring new metadata entry introduced in newer version. which it would be much rollback friendly.
I don't understand the concern here. Header version is an integer on disk. V0 version has value '0' which correspond with the zeroed entry logger current headers. newer entry logs will have version 1. Old code is anyway ignoring the header version here, so we will be able to rollback.
> On April 14, 2015, 7:37 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, line 972
> > <https://reviews.apache.org/r/33061/diff/1/?file=922608#file922608line972>
> >
> > we shouldn't limit version. for future version upgrade.
This check is in the extractEntryLogMetadataFromIndex() method called explicitely to read the index. If the Version is below V1, it means we have no index to read from anyway.
If the HeaderVersion is later increased to V2, V3... This check will still be valid: ledgersMap index has been added in V1, so it will not be present in any prior versions.
> On April 14, 2015, 7:37 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, line 907
> > <https://reviews.apache.org/r/33061/diff/1/?file=922608#file922608line907>
> >
> > I don't think you need to limit maxPosition here, because the ledgers map entries will be skipped anyway.
True, I'll change that.
> On April 14, 2015, 7:37 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, line 133
> > <https://reviews.apache.org/r/33061/diff/1/?file=922608#file922608line133>
> >
> > I think you need to add a 4 bytes integer for total number of ledgers in the entry logger. so it would prevent any missing metadata entry causing missing ledgers.
> >
> > ledger map offset: 8 bytes
> > total number of ledgers: 4 bytes
Ok, so if we don't find all the metadata entries, we'll fall back to scanning.
> On April 14, 2015, 7:37 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, line 426
> > <https://reviews.apache.org/r/33061/diff/1/?file=922608#file922608line426>
> >
> > could you move #appendLedgerMap() to flushRotatedLogs()? so appendLedgerMap happens only on checkpointing, rather than when rolling log. because rolling log happens in critical path which will impact latency.
The problem is that on flushRotatedLogs() we have a list of BufferedLogChannel. I'll change it to keep the EntryLogMetadata in the BufferedLogChannel, so we can have multiple outstanding to be flushed.
> On April 14, 2015, 7:37 a.m., Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, line 447
> > <https://reviews.apache.org/r/33061/diff/1/?file=922608#file922608line447>
> >
> > append ledger map, should be appending multiple ledger maps metadata entries. rather than dumping into one big entry.
> >
> > --> (offset of ledgers map)
> > - <ledgers_map_entry1>
> > - <ledgers_map_entry2>
> > - ...
> > - <ledgers_map_entry3>
While I agree that we should design it to be easily extensible, I fail to see the benefit, on current conditions, to split the map on multiple entries when writing.
The map is already in memory and we need to put it on disk. When we'll read the map, we'll have to read it entirely in mem again.
Also, the number of ledgers a bookie can host is anyway kind of limited. Above few tens of thousands of ledgers, the bookies performance is severely impacted.
Even considering 100K ledgers in bookie, all of which with entries written in the current entry log, the ledgersMap will be 1.5Mb.
I'll add a max batch size of 10K ledgers, that would mean a max entry size of 160Kb.
- Matteo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33061/#review79997
-----------------------------------------------------------
On April 10, 2015, 3:41 a.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33061/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 3:41 a.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-841
> https://issues.apache.org/jira/browse/BOOKKEEPER-841
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Bookie should calculate ledgers map when writing a new entry log file. so the bookie doesn't need to scan that entry log file again, which it would improve garbage collection efficiency
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 0e052b5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java 488f4bf
>
> Diff: https://reviews.apache.org/r/33061/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>
Re: Review Request 33061: BOOKKEEPER-841: Bookie should calculate
ledgers map writing a new entry log file
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33061/#review79997
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java
<https://reviews.apache.org/r/33061/#comment129731>
make it private
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java
<https://reviews.apache.org/r/33061/#comment129732>
remove trailing spaces?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java
<https://reviews.apache.org/r/33061/#comment129733>
remove trailing spaces?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java
<https://reviews.apache.org/r/33061/#comment129734>
remove trailing spaces?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java
<https://reviews.apache.org/r/33061/#comment129735>
remove trailing spaces?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment129736>
I don't prefer using a int here. so for format upgrading, the old code could still read new version format by ignoring new metadata entry introduced in newer version. which it would be much rollback friendly.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment129737>
trailing space?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment129738>
I think you need to add a 4 bytes integer for total number of ledgers in the entry logger. so it would prevent any missing metadata entry causing missing ledgers.
ledger map offset: 8 bytes
total number of ledgers: 4 bytes
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment129739>
trailing space?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment129741>
could you move #appendLedgerMap() to flushRotatedLogs()? so appendLedgerMap happens only on checkpointing, rather than when rolling log. because rolling log happens in critical path which will impact latency.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment129743>
append ledger map, should be appending multiple ledger maps metadata entries. rather than dumping into one big entry.
--> (offset of ledgers map)
- <ledgers_map_entry1>
- <ledgers_map_entry2>
- ...
- <ledgers_map_entry3>
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment129744>
remove this. to make it rollback friendly. as what the comment for HeaderVersion
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment129746>
I don't think you need to limit maxPosition here, because the ledgers map entries will be skipped anyway.
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
<https://reviews.apache.org/r/33061/#comment129747>
we shouldn't limit version. for future version upgrade.
- Sijie Guo
On April 10, 2015, 3:41 a.m., Matteo Merli wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33061/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 3:41 a.m.)
>
>
> Review request for bookkeeper.
>
>
> Bugs: BOOKKEEPER-841
> https://issues.apache.org/jira/browse/BOOKKEEPER-841
>
>
> Repository: bookkeeper-git
>
>
> Description
> -------
>
> Bookie should calculate ledgers map when writing a new entry log file. so the bookie doesn't need to scan that entry log file again, which it would improve garbage collection efficiency
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogMetadata.java PRE-CREATION
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 0e052b5
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 299fb3e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java 0a3884c
> bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java 488f4bf
>
> Diff: https://reviews.apache.org/r/33061/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Matteo Merli
>
>