> 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 > >
