Supporting both configs at the same time to begin with would allow enough time for users to experiment with the settings. If in the future its deemed that a memory bound is far superior to a #req bound (which is my opinion but needs evidence to support) the migration discussion could be deferred to that point, making immediate impact tiny (conf upgrade is trivial, behavior remains backwards compatible, and new feature is "opt-in").
I will extend the Unit tests (and fix the bugs) as part of the implementation. As for the performance implications: 1. my initial runs (https://github.com/radai-rosenblatt/kafka-benchmarks) show ~5Mops/sec using the current queue (ArrayBlockingQueueBenchmark) and ~400Kops/sec using the proposed queue datastructure (ByteBoundedBlockingQueueBenchmark). this is using 16 producer threads and 24 consumer threads on my machine. 2. ByteBoundedBlockingQueue uses composition on top of ArrayBlockingQueue, since ArrayBlockingQueue is not easily extendable. this leads to the double locking you mentioned. an alternative would be to basically copy-paste ArrayBlockingQueue and add the functionality (as done in https://github.com/radai-rosenblatt/kafka-benchmarks/blob/master/src/main/java/net/radai/kafka/MetricBoundedArrayBlockingQueue.java). this shown no noticeable impact on queue performance (also covered in my benchmark code) but is far less elegant. 3. 400Kops/sec is still more than enough for realistic kafka installations, and the effect can be further mitigated by using ByteBoundedBlockingQueue only if the configuration was set (so no impact if the feature is "opt in" on default installations) 4. As part of the implementation I will also deploy the proposed code in a test env here (linkedin) and report back on measured throughput vs a vanilla version On Fri, Aug 5, 2016 at 8:38 AM, Ismael Juma <ism...@juma.me.uk> wrote: > Hi Radai, > > Thanks for the proposal. I think it makes sense to be able to limit the > request queue by bytes. I haven't made up my mind on whether having both > limits is better than having a single one yet, but we probably need to make > a call on that before we can start a vote. > > Just a quick point with regards to ByteBoundedBlockingQueue. It's not > currently used in the code and the existing unit tests are very basic. > Also, looking at the implementation, it has two obvious bugs (it fails if > one passes None for `sizeFunction` and `currentByteSize` is not final and > it should be to comply with the JMM). So, we probably need to extend the > unit tests for that class if we want to use it (ideally there would be > multi-threaded tests too). It also is a bit inefficient as its adds > additional locks on top of the LinkedBlockingQueue ones, but the benchmarks > should hopefully show if that is something that needs to be addressed or > not. > > Ismael > > On Thu, Aug 4, 2016 at 7:28 PM, radai <radai.rosenbl...@gmail.com> wrote: > > > Hello, > > > > I'd like to initiate a discussion about > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > 72%3A+Allow+Sizing+Incoming+Request+Queue+in+Bytes > > > > The goal of the KIP is to allow configuring a bound on the capacity (as > in > > bytes of memory used) of the incoming request queue, in addition to the > > current bound on the number of messages. > > > > This comes after several incidents at Linkedin where a sudden "spike" of > > large message batches caused an out of memory exception. > > > > Thank you, > > > > Radai > > >
