> On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > 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.
One of the reasons to put this in the same patch was to make sure that things don't get committed out of order. We definitely don't want the multiple thread patch to go in before we make everything else threadsafe. I could generate separate diffs for these if you feel strongly about it. However, it should be possible to review each component separately because the changes make the components thread safe. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java, > > line 705 > > <https://reviews.apache.org/r/7560/diff/2/?file=176097#file176097line705> > > > > Why not perform masterKeyCache#putIfAbsent here? so we only allocate > > ByteBuffer one time the one who put the master key succeed. I do a putIfAbsent later after first checking if the value exists to avoid allocating too many ByteBuffers (one on every call) > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedReadChannel.java, > > line 1 > > <https://reviews.apache.org/r/7560/diff/2/?file=176099#file176099line1> > > > > 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? I agree. This was in my TODOs but forgot to include it :( > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, > > line 37 > > <https://reviews.apache.org/r/7560/diff/2/?file=176100#file176100line37> > > > > Is there any consideration you change to import *? No, I'll remove the import *. IntelliJ did this. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, > > line 255 > > <https://reviews.apache.org/r/7560/diff/2/?file=176100#file176100line255> > > > > 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. I don't quite understand. readEntry() in EntryLogger will open a file handle to the log file on the first read. This handle is a thread local variable, so every thread will have a buffered read channel for every log file. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, > > line 268 > > <https://reviews.apache.org/r/7560/diff/2/?file=176100#file176100line268> > > > > 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? I know this leaks file handles. I think the only way to fix this would be to remove the thread local and maintain a thread-><logid, bufferedreadchannel> map in the class. The problem here is that the bufferedreadchannels will not be garbage collected even if i close the file handle. I'll close the file handle here so at least fileids will not be leaked. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, > > line 556 > > <https://reviews.apache.org/r/7560/diff/2/?file=176100#file176100line556> > > > > the original code would close the writing channel also. but your code > > just close reading channels. Thanks. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java, > > line 566 > > <https://reviews.apache.org/r/7560/diff/2/?file=176100#file176100line566> > > > > same issue as above. Thanks for pointing this out. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java, > > line 63 > > <https://reviews.apache.org/r/7560/diff/2/?file=176105#file176105line63> > > > > it would be better to put these settings in bk_server.conf. Will do. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/MultiPacketProcessor.java, > > line 78 > > <https://reviews.apache.org/r/7560/diff/2/?file=176107#file176107line78> > > > > 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. Sounds good. Will fix. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/ReadEntryProcessor.java, > > line 68 > > <https://reviews.apache.org/r/7560/diff/2/?file=176110#file176110line68> > > > > 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. I'll add all the log messages again. Sorry about that. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/WriteEntryProcessor.java, > > line 1 > > <https://reviews.apache.org/r/7560/diff/2/?file=176111#file176111line1> > > > > A better name for this class would be 'AddEntryProcessor' to keep > > consistency with the operation name. Agreed. Will fix. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookieReadWriteTest.java, > > line 256 > > <https://reviews.apache.org/r/7560/diff/2/?file=176113#file176113line256> > > > > 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. Yeah, this test is not really needed. Will remove. > On Oct. 16, 2012, 7:57 a.m., Sijie Guo wrote: > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/MultipleThreadReadTest.java, > > line 1 > > <https://reviews.apache.org/r/7560/diff/2/?file=176114#file176114line1> > > > > 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? This is not a performance test. I wanted to test if the concurrent reads and writes work correctly. This test handles that. - Aniruddha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7560/#review12470 ----------------------------------------------------------- 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 > >
