> On May 19, 2015, 7:12 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java, > > line 498 > > <https://reviews.apache.org/r/33096/diff/1/?file=923970#file923970line498> > > > > why not create the sorted ledger storage also by reflection?
I didn't want to break configuration compatibility. Today if you specify sortedLedgerStorageEnabled=true, that means to instantiate a different class. We can deprecate the sortedLedgerStorageEnabled option, or remove it and set the SortedLedgerStorage as the default implementation. > On May 19, 2015, 7:12 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java, > > line 122 > > <https://reviews.apache.org/r/33096/diff/1/?file=923971#file923971line122> > > > > the comment isn't accurate. it isn't syncing the underlying storage > > though. After the compaction is done, all the indexes are updated and they are indeed fsynced before the old entryLog can be deleted, otherwise we might lose the access the compacted entries. I'll change the comment to "sync the underlying *index*" > On May 19, 2015, 7:12 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java, > > line 424 > > <https://reviews.apache.org/r/33096/diff/1/?file=923971#file923971line424> > > > > this changes the behavior here. it doesn't need to load file info to > > file info cache before. but right now, it would, which it would pollute the > > file info cache and evict other active file infos. > > > > you might consider adding a separate interface for > > CompatableLedgerStorage. That's true, though I don't see why `IndexPersistenceMgr.ledgerExist()` cannot be simplyfied into: ``` boolean ledgerExists(long ledgerId) throws IOException { return activeLedgers.containsKey(ledgerId); } ``` > On May 19, 2015, 7:12 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java, > > line 245 > > <https://reviews.apache.org/r/33096/diff/1/?file=923971#file923971line245> > > > > this seems incorrect. you'd better to have a separate method > > 'syncEntriesLocations', which differentiate 'updateEntriesLocations' from > > 'syncEntriesLocations'. OK, I see your point, this code is actually flushing the indexes multiple times during compaction. Fixed it. > On May 19, 2015, 7:12 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java, > > line 85 > > <https://reviews.apache.org/r/33096/diff/1/?file=923976#file923976line85> > > > > why change this logic here? It'd better to not to change it if it isn't > > a problem. Reverted to original logic - Matteo ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33096/#review84268 ----------------------------------------------------------- On April 10, 2015, 10:18 p.m., Matteo Merli wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33096/ > ----------------------------------------------------------- > > (Updated April 10, 2015, 10:18 p.m.) > > > Review request for bookkeeper. > > > Bugs: BOOKKEEPER-851 > https://issues.apache.org/jira/browse/BOOKKEEPER-851 > > > Repository: bookkeeper-git > > > Description > ------- > > Allow to configure a different implementation for the LedgerStorage interface > in the Bookie configuration. > > > Diffs > ----- > > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookKeeperServerStats.java > 9036182 > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java > 8b40853 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java > 299fb3e > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java > 1a69692 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorage.java > e992d03 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerStorageFactory.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ReadOnlyEntryLogger.java > a2ce9e3 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ScanAndCompareGarbageCollector.java > d8a87e4 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/SortedLedgerStorage.java > 0a3884c > > bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java > cd9f7a0 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java > 6c9c4a7 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/TestSyncThread.java > d947dff > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java > 19aab44 > > Diff: https://reviews.apache.org/r/33096/diff/ > > > Testing > ------- > > > Thanks, > > Matteo Merli > >
