----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3874/#review5162 -----------------------------------------------------------
In general the patch is good, but there's a few mods i'd like to see. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/3874/#comment11305> EntryLogMeta or EntryLogMetadata would be a better name as it only refers to a single log of the entry logger. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/3874/#comment11307> Would it not be better to loop over entrySet rather than keySet here? for (Map.Entry<Long, Etc> e : entryLogs2LedgersMap.entrySet()) { } bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/3874/#comment11310> 1024*1024 should be defined as a constant somewhere bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/3874/#comment11309> Shouldn't you throw an exception here? bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/3874/#comment11308> 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. bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java <https://reviews.apache.org/r/3874/#comment11298> The defaults for the new options should be added to bk_server.conf in a commented out fashion like other options. bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java <https://reviews.apache.org/r/3874/#comment11311> 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. - Ivan 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 > >
