> On 2012-02-16 17:43:31, Ivan Kelly wrote: > > In general the patch is good, but there's a few mods i'd like to see.
thanks Ivan for reviewing it. I will fix the issues as comment and attach a new patch. > On 2012-02-16 17:43:31, Ivan Kelly wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, > > line 739 > > <https://reviews.apache.org/r/3874/diff/1/?file=74506#file74506line739> > > > > I don't like how EntryLogger is reaching back into Bookie to write the > > entries. Also, I don't think it's necessary. > > > > How I had imagined this working is that you create a new entry log to > > compact into. You copy the entries across and then rename the file to the > > name of the old entry log. it is not safe to do as what you described. since after compaction, the position of entries has been changed and we should let this kind of change be reflected in the index file, otherwise we would lost it. The safe way is to let the compacted entries go thru again the addEntry flow to apply the position change to index file. > On 2012-02-16 17:43:31, Ivan Kelly wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java, > > line 412 > > <https://reviews.apache.org/r/3874/diff/1/?file=74507#file74507line412> > > > > I think we should run compaction based purely on the watermarks. > > Otherwise, once we hit disk pressure, we'll have to run a lot of compaction > > at once. what I am thinking is that we need to a setting to let user control whether to enable compaction or not and when to do compaction. e.g, user can configuration the threshold to the size to half of the capacity of a disk, the compaction is enabled when the capacity reaches this threshold. since compaction is expensive to move entries. - Sijie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3874/#review5162 ----------------------------------------------------------- On 2012-02-12 08:47:34, Sijie Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3874/ > ----------------------------------------------------------- > > (Updated 2012-02-12 08:47:34) > > > Review request for bookkeeper. > > > Summary > ------- > > bookie server aggregates entries into entry log file. suppose there is lots > of ledgers, each ledger has little messages. so a entry log file would > contains messages from lots of different ledgers. if there is only one ledger > not be deleted, the entry log file would not be removed, whose occupied disk > space could not be reclaimed. > > > This addresses bug BOOKKEEPER-160. > https://issues.apache.org/jira/browse/BOOKKEEPER-160 > > > Diffs > ----- > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java > 57a6c29 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java > 42f54d2 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java > 0c83977 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java > PRE-CREATION > > bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java > 2e5d784 > > Diff: https://reviews.apache.org/r/3874/diff > > > Testing > ------- > > > Thanks, > > Sijie > >
