----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7560/#review13212 -----------------------------------------------------------
I think this should be split into at least 3 separate changes. The BufferedChannel changes, the LedgerCache changes, and the NIOServer changes. And perhaps another misc jira for thread safety issues that dont fall into those categories. Regarding application order of the patches, jira lets you specify that a jira is dependent on another. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java <https://reviews.apache.org/r/7560/#comment28419> the log needs to be written to the journal before putting the key in the map. Otherwise, if a concurrent thread comes along and sees the key in the map, before is has been added to the journal, it can add an entry for that ledger, which will be unrecoverable. Im not sure what the correct solution is. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java <https://reviews.apache.org/r/7560/#comment28420> Don't put TODOs without corresponding jiras. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java <https://reviews.apache.org/r/7560/#comment28421> These BufferedChannel changes look like they could be spun out into a separate patch. Please do, as it makes reviewing much easier. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/7560/#comment28423> Seems like it would make sense to have some ref counting on the file channels, so that they can be shared, by the BufferedReadChannels? bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/7560/#comment28422> should remove this before submission. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/7560/#comment28424> This will only close the entrylogs open on one of the threads. - Ivan Kelly On Oct. 11, 2012, 11:22 p.m., Aniruddha Laud wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7560/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2012, 11:22 p.m.) > > > Review request for bookkeeper, fpj, Uma Maheswara Rao G, Ivan Kelly, and > Rakesh R. > > > Description > ------- > > The current bookkeeper server is single threaded. The same thread handles > reads and writes. When reads are slow (possibly because of excessive seeks), > add entry operations suffer in terms of latencies. Providing separate read > and write threads helps in reducing add entry latencies and increasing > throughput even when we're facing slow reads. Having a single read thread > also results in low disk utilization because seeks can't be ordered > efficiently by the OS. Multiple read threads would help in improving the read > throughput. > > > This addresses bug BOOKKEEPER-429. > https://issues.apache.org/jira/browse/BOOKKEEPER-429 > > > Diffs > ----- > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java > 9a62264 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java > e2260fd > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java > 87b0c66 > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfo.java > 77e08bf > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java > 558e8cf > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCacheImpl.java > 3c39404 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerEntryPage.java > 8395a2f > > bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java > 4af5dec > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java > 2b7466f > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/MultiPacketProcessor.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/NIOServerFactory.java > 05acacf > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PacketProcessorBase.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java > PRE-CREATION > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java > PRE-CREATION > > bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java > 3d59857 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieReadWriteTest.java > dc5eefa > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultipleThreadReadTest.java > PRE-CREATION > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/NIOServerFactoryTest.java > bdcb7a2 > > Diff: https://reviews.apache.org/r/7560/diff/ > > > Testing > ------- > > > Thanks, > > Aniruddha Laud > >
