Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

JIRA up - https://issues.apache.org/jira/browse/KAFKA-4602 PR up - https://github.com/apache/kafka/pull/2330 KIP wiki has been updated. On Fri, Jan 6, 2017 at 8:16 AM, radai wrote: > Will do (sorry for the delay). > and thank you. > > On Fri, Jan 6, 2017 at 7:56 AM, Ismael Juma wrote: > >> Ra

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Will do (sorry for the delay). and thank you. On Fri, Jan 6, 2017 at 7:56 AM, Ismael Juma wrote: > Radai, you have more than enough votes to declare the vote successful. > Maybe it's time to do so. :) Also, once you have done that, it would be > good to move this KIP to the adopted table in the

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Radai, you have more than enough votes to declare the vote successful. Maybe it's time to do so. :) Also, once you have done that, it would be good to move this KIP to the adopted table in the wiki. Thanks! Ismael On Fri, Jan 6, 2017 at 2:10 AM, Jun Rao wrote: > Hi, Radai, > > The new metrics

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hi, Radai, The new metrics look good. +1 on the KIP. Thanks, Jun On Fri, Dec 16, 2016 at 4:46 PM, radai wrote: > I've added the 3 new metrics/sensors i've implemented to the KIP. > > at this point I would need to re-validate the functionality (which i expect > to do early january). > > code r

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

+1 on the KIP. I'll comment more on the formal PR. Also, can you also link a jira for this from the KIP? Thanks, Joel On Tue, Jan 3, 2017 at 11:14 AM, radai wrote: > I've just re-validated the functionality works - broker throttles under > stress instead of OOMs. > > at this point my branch (

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

I've just re-validated the functionality works - broker throttles under stress instead of OOMs. at this point my branch ( https://github.com/radai-rosenblatt/kafka/tree/broker-memory-pool-with-muting) is "code complete" and somewhat tested and im waiting on the voting process to come to a conclusi

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

I've added the 3 new metrics/sensors i've implemented to the KIP. at this point I would need to re-validate the functionality (which i expect to do early january). code reviews welcome ;-) On Mon, Nov 28, 2016 at 10:37 AM, radai wrote: > will do (only added a single one so far, the rest TBD) >

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

will do (only added a single one so far, the rest TBD) On Mon, Nov 28, 2016 at 10:04 AM, Jun Rao wrote: > Hi, Radai, > > Could you add a high level description of the newly added metrics to the > KIP wiki? > > Thanks, > > Jun > > On Wed, Nov 23, 2016 at 3:45 PM, radai wrote: > > > Hi Jun, > > >

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hi, Radai, Could you add a high level description of the newly added metrics to the KIP wiki? Thanks, Jun On Wed, Nov 23, 2016 at 3:45 PM, radai wrote: > Hi Jun, > > I've added the sensor you requested (or at least I think I did ) > > On Fri, Nov 18, 2016 at 12:37 PM, Jun Rao wrote: > >

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Thanks for the KIP, +1 from me. I have a few comments with regards to the method names chosen, but since none of the classes in question are public API, I'll comment directly in the PR. Ismael On Mon, Nov 7, 2016 at 9:08 PM, radai wrote: > Hi, > > I would like to initiate a vote on KIP-72: > >

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hi Jun, I've added the sensor you requested (or at least I think I did ) On Fri, Nov 18, 2016 at 12:37 PM, Jun Rao wrote: > KafkaRequestHandlerPool

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hi, Radai, 3. Having a gauge of MemoryAvailable is useful. One issue with that though is that if one only collects the metrics say every minute, one doesn't know what has happened in between. We could additionally track the fraction of the time when requested memory can't be served. Every time a r

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hi Jun, 3. will (also :-) ) do. do you have ideas for appropriate names/metrics? I'm thinking along the lines of "MemoryAvailable" (current snapshot value from pool) and "Throttles" (some moving-avg of how often does throttling due to no mem kicks in). maybe also "BuffersOutstanding" ? On Thu, No

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hi, Radai, 2. Yes, on the server side, the timeout is hardcoded at 300ms. That's not too bad. We can just leave it as it is. 3. Another thing. Do we plan to expose some JMX metrics so that we can monitor if there is any memory pressure in the pool? Thanks, Jun On Thu, Nov 17, 2016 at 8:57 AM,

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hi Jun, 1. will do. 2. true. for several reasons: 2.1. which selector? there's a single pool but 16 selectors (linkedin typical, num.network.threads defaults to 3) 2.2. even if i could figure out which selector (all?) the better thing to do would be resume reading not when any memory become

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hi, Radai, Thanks for the updated proposal. +1 overall. A couple of comments below. 1. Our current convention is to avoid using getters. Could you change getSize and getAvailableMemory accordingly? Also, size is bit ambiguous, could we use sth like capacity? 2. This is more on the implementation

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

+1 Thank you for the KIP, Radai. On Mon, Nov 14, 2016 at 6:07 PM, Mickael Maison wrote: > +1. We've also been hit by OOMs on the broker because we were not able > to properly bound its memory usage. > > On Mon, Nov 14, 2016 at 5:56 PM, radai wrote: > > @rajini - fixed the hasBytesBuffered() me

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

+1. We've also been hit by OOMs on the broker because we were not able to properly bound its memory usage. On Mon, Nov 14, 2016 at 5:56 PM, radai wrote: > @rajini - fixed the hasBytesBuffered() method. also updated poll() so that > no latency is added for picking up data stuck in ssl buffers (tim

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

@rajini - fixed the hasBytesBuffered() method. also updated poll() so that no latency is added for picking up data stuck in ssl buffers (timeout is set to 0, just like with immediately connected keys and staged receives). thank you for pointing these out. added ssl (re) testing to the KIP testing p

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Open point 1. I would just retain the current long value that specifies queued.max.bytes as long and not as %heap since it is simple and easy to use. And keeps it consistent with other ".bytes" configs. Point 3. ssl buffers - I am not quite sure the implementation looks correct. hasBytesBuffered()

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

ok, i've made the following changes: 1. memory.pool.class.name has been removed 2. the code now only uses SimpleMemoryPool. the gc variant is left (unused) as a developement aid and is unsettable via configuration. 3. I've resolved the issue of stale data getting stuck in intermediate (ssl) buffer

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

13. At the moment, I think channels are not muted if: channel.receive != null && channel.receive.buffer != null This mutes all channels that aren't holding onto a incomplete buffer. They may or may not have read the 4-byte size. I was thinking you could avoid muting channels if: channel.re

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hey Radai, +1 on deprecating and eventually removing the old config. The intention was absolutely bounding memory usage. I think having two ways of doing this, one that gives a crisp bound on memory and one that is hard to reason about is pretty confusing. I think people will really appreciate hav

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

jun's #1 + rajini's #11 - the new config param is to enable changing the pool implentation class. as i said in my response to jun i will make the default pool impl be the simple one, and this param is to allow a user (more likely a dev) to change it. both the simple pool and the "gc pool" make basi

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Radai, 11. The KIP talks about a new server configuration parameter *memory.pool.class.name *which is not in the implementation. Is it still the case that the pool will be configurable? 12. Personally I would prefer not to have a garbage collected pool that hides b

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hi, Radai, 1. Yes, I am concerned about the trickiness of having to deal with wreak refs. I think it's simpler to just have the simple version instrumented with enough debug/trace logging and do enough stress testing. Since we still have queued.max.requests, one can always fall back to that if a m

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hi Jun, Thank you for taking the time to review this. 1. short version - yes, the concern is bugs, but the cost is tiny and worth it, and its a common pattern. long version: 1.1 detecting these types of bugs (leaks) cannot be easily done with simple testing, but requires stress/stability tests

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

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

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

+1 (binding) On Tue, Nov 8, 2016 at 10:26 AM, radai wrote: > I've updated the KIP page to specify the new config would co-exist with > "queued.max.request" to minimize the impact on compatibility. > > On Tue, Nov 8, 2016 at 7:02 AM, radai wrote: > >> My personal opinion on this is that control o

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

I've updated the KIP page to specify the new config would co-exist with "queued.max.request" to minimize the impact on compatibility. On Tue, Nov 8, 2016 at 7:02 AM, radai wrote: > My personal opinion on this is that control of memory was always the > intent behind queued.max.requests and so thi

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

My personal opinion on this is that control of memory was always the intent behind queued.max.requests and so this KIP could completely obsolete it. For now its probably safest to leave it as-is (making memory-bound "opt-in") and revisit this at a later date On Mon, Nov 7, 2016 at 2:32 PM, Gwen Sh

Re: [VOTE] KIP-72 - Allow putting a bound on memory consumed by Incoming requests

Hey Radai, Looking at the proposal, it looks like a major question is still unresolved? "This configuration parameter can either replace queued.max.requests completely, or co-exist with it (by way of either-or or respecting both bounds and not picking up new requests when either is hit)." On Mon,