You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Aniruddha Laud <i0...@yahoo.com> on 2012/10/12 01:22:47 UTC
Review Request: Provide separate read and write threads in the bookkeeper
server
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7560/
-----------------------------------------------------------
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
Re: Review Request: Provide separate read and write threads in the
bookkeeper server
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: Provide separate read and write threads in the
bookkeeper server
Posted by Sijie Guo <gu...@gmail.com>.
> 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
>
>
Re: Review Request: Provide separate read and write threads in the
bookkeeper server
Posted by Aniruddha Laud <i0...@yahoo.com>.
> 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
>
>
Re: Review Request: Provide separate read and write threads in the
bookkeeper server
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
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
>
>