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