> 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. > > Aniruddha Laud wrote: > 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.
One question, is LedgerCache thread-safe before your changes? In my mind, it should be thread-safe. If so, how about moving LedgerCache changes to a separated JIRA? > 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? > > Aniruddha Laud wrote: > This is not a performance test. I wanted to test if the concurrent reads > and writes work correctly. This test handles that. OK. How about using name 'ConcurrentReadWriteTest'? since what you want to test is not read only but also writes. > 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. > > Aniruddha Laud wrote: > I do a putIfAbsent later after first checking if the value exists to > avoid allocating too many ByteBuffers (one on every call) yes. my point is that could we move putIfAbsent to here. like if (null == masterKeyCache.putIfAbsent(ledgerId, masterKey)) { // log add operation. } > 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. > > Aniruddha Laud wrote: > 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. > yes. ur right. seems OK for me. > 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? > > Aniruddha Laud wrote: > 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. I am just thinking that do we need a BufferedReadChannel? could we use a FileChannel directly? since reads are random, an application level buffer would not help a lot. OS's filesystem already does a lot on caching. I think reading directly from a FileChannel for random reads would be OK. If that, we don't need the thread local map. It could simplify the changes a lot. - Sijie ----------------------------------------------------------- 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 > >
