----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7560/#review12470 -----------------------------------------------------------
the overview of the patch looks good. but seems the patch moved and modified the code at the same time, it is quite hard to verify the code move itself is correct or not. I would suggest that splitting this patch into smaller pieces, 1) is to produced the processor staffs (code moved), 2) is to make LedgerCache, EntryLogger to be thread-safe, which could be executed from multiple threads, 3) is to introduce the multiplepacket processor. >> LedgerCache changes In my mind, LedgerCache would be thread-safe to access from different thread. Seems the changes in LedgerCache is to improve it using concurrent data structures. If it was that, I would suggest that it would be a separated JIRA to make things more clearly. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java <https://reviews.apache.org/r/7560/#comment26528> Why not perform masterKeyCache#putIfAbsent here? so we only allocate ByteBuffer one time the one who put the master key succeed. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java <https://reviews.apache.org/r/7560/#comment26539> since this file shared some code with BufferedChannel. so I would suggest that we could leverage it. how about renaming BufferedChannel to BufferedReadWriteChannel, and let BufferedReadWriteChannel extends BufferedReadChannel? bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/7560/#comment26529> Is there any consideration you change to import *? bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/7560/#comment26540> you removed channels mapping, so for those written entry log files, you don't use them after creating a new entry log file. there is a race here: (N+1) log created to serving writes, but (N) log doesn't exited in logid2filechannel util it was put in logid2filechannel when scanning entry log files. If read requests entering at this time, it would fail. I would suggested for those written entry log files, we keep using the opened channel. if there is no channel in it, we created a readonly channel for it. the change would be simple: we could have a BufferedChannel interface and a ReadOnly and a ReadWrite implementations for it. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/7560/#comment26536> the original code here is to close the underlying channel so the space occupied by the deleted file could be reclaimed. but the code you changed did nothing. any consideration? bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/7560/#comment26537> the original code would close the writing channel also. but your code just close reading channels. bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java <https://reviews.apache.org/r/7560/#comment26538> same issue as above. bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java <https://reviews.apache.org/r/7560/#comment26535> it would be better to put these settings in bk_server.conf. bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/MultiPacketProcessor.java <https://reviews.apache.org/r/7560/#comment26530> why not validate version here? since we already get header from the packet. otherwise, we had to write version validation for each type of requests. bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java <https://reviews.apache.org/r/7560/#comment26534> seems that you removed the log messages when moving the code. since the patch moved and modified the code at the same time, it is quite hard to verify the code move itself is correct or not. I would suggest that splitting this patch into smaller pieces, 1) is to produced the processor staffs (code moved), 2) is to make LedgerCache, EntryLogger to be thread-safe, which could be executed from multiple threads, 3) is to introduce the multiplepacket processor. bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java <https://reviews.apache.org/r/7560/#comment26531> A better name for this class would be 'AddEntryProcessor' to keep consistency with the operation name. bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieReadWriteTest.java <https://reviews.apache.org/r/7560/#comment26532> what kind of case that testReadWriteAsyncSingleClient200 could not cover? If not, I don't think we don't need to add another case just testing with more entries, since it would introduce more time to run the test cases. bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultipleThreadReadTest.java <https://reviews.apache.org/r/7560/#comment26533> I am assuming the test cases in test package are used to test features and correctness. Do we really need a case to cover performance testing? - Sijie Guo 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 > >
