Hi, Radai, Thanks for the KIP. Some comments below.
1. The KIP says "to facilitate faster implementation (as a safety net) the pool will be implemented in such a way that memory that was not release()ed (but still garbage collected) would be detected and "reclaimed". this is to prevent "leaks" in case of code paths that fail to release() properly.". What are the cases that could cause memory leaks? If we are concerned about bugs, it seems that it's better to just do more testing to make sure the usage of the simple implementation (SimpleMemoryPool) is solid instead of adding more complicated logic (GarbageCollectedMemoryPool) to hide the potential bugs. 2. I am wondering how much this KIP covers the SSL channel implementation. 2.1 SslTransportLayer maintains netReadBuffer, netWriteBuffer, appReadBuffer per socket. Should those memory be accounted for in memory pool? 2.2 One tricky thing with SSL is that during a KafkaChannel.read(), it's possible for multiple NetworkReceives to be returned since multiple requests' data could be encrypted together by SSL. To deal with this, we stash those NetworkReceives in Selector.stagedReceives and give it back to the poll() call one NetworkReceive at a time. What this means is that, if we stop reading from KafkaChannel in the middle because memory pool is full, this channel's key may never get selected for reads (even after the read interest is turned on), but there are still pending data for the channel, which will never get processed. 3. The code has the following two methods in MemoryPool, which are not described in the KIP. Could you explain how they are used in the wiki? isLowOnMemory() isOutOfMemory() 4. Could you also describe in the KIP at the high level, how the read interest bit for the socket is turned on/off with respect to MemoryPool? 5. Should queued.max.bytes defaults to -1 or Long.MAX_VALUE? Thanks, Jun On Mon, Nov 7, 2016 at 1:08 PM, radai <radai.rosenbl...@gmail.com> wrote: > Hi, > > I would like to initiate a vote on KIP-72: > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-72%3A+ > Allow+putting+a+bound+on+memory+consumed+by+Incoming+requests > > The kip allows specifying a limit on the amount of memory allocated for > reading incoming requests into. This is useful for "sizing" a broker and > avoiding OOMEs under heavy load (as actually happens occasionally at > linkedin). > > I believe I've addressed most (all?) concerns brought up during the > discussion. > > To the best of my understanding this vote is about the goal and > public-facing changes related to the new proposed behavior, but as for > implementation, i have the code up here: > > https://github.com/radai-rosenblatt/kafka/tree/broker-memory > -pool-with-muting > > and I've stress-tested it to work properly (meaning it chugs along and > throttles under loads that would DOS 10.0.1.0 code). > > I also believe that the primitives and "pattern"s introduced in this KIP > (namely the notion of a buffer pool and retrieving from / releasing to said > pool instead of allocating memory) are generally useful beyond the scope of > this KIP for both performance issues (allocating lots of short-lived large > buffers is a performance bottleneck) and other areas where memory limits > are a problem (KIP-81) > > Thank you, > > Radai. >
