I had some offline discussions with Lucas on this KIP. While it is much more work than the original proposals, separating the control plane entirely removes any interference with the data plane as summarized under the rejected alternatives section.
Just a few minor comments: - Can you update the link to the discussion thread and vote thread? - The idle ratio metrics are fairly important for monitoring. I think we agreed that these would only apply to the data plane (otherwise there will always be some skew due to the controller plane). If so, can you clarify that somewhere in the doc? - Personally, I prefer the term CONTROL to CONTROLLER in the configs. CONTROLLER makes it sound like it is a special listener on the controller. CONTROL clarifies that this is a listener for receiving control plane requests from the controller. Thanks, Joel On Wed, Aug 22, 2018 at 12:45 AM, Eno Thereska <eno.there...@gmail.com> wrote: > Ok thanks, if you guys are seeing this at LinkedIn then the motivation > makes more sense. > > Eno > > On Tue, Aug 21, 2018 at 5:39 PM, Becket Qin <becket....@gmail.com> wrote: > > > Hi Eno, > > > > Thanks for the comments. This KIP is not really about improving the > > performance in general. It is about ensuring the cluster state can still > be > > updated quickly even if the brokers are under heavy load. > > > > We have seen quite often that it took dozens of seconds for a broker to > > process the requests sent by the controller when the cluster is under > heavy > > load. This leads to the issues Lucas mentioned in the motivation part. > > > > Thanks, > > > > Jiangjie (Becket) Qin > > > > > On Aug 20, 2018, at 11:33 PM, Eno Thereska <eno.there...@gmail.com> > > wrote: > > > > > > Hi folks, > > > > > > I looked at the previous numbers that Lucas provided (thanks!) but it's > > > still not clear to me whether the performance benefits justify the > added > > > complexity. I'm looking for some intuition here (a graph would be great > > but > > > not required): for a small/medium/large cluster, what are the expected > > > percentage of control requests today that will benefit from the change? > > > It's a bit hard to go through this level of detail without knowing the > > > expected end-to-end benefit. The best folks to answer this might be > ones > > > running such clusters, and ideally should pitch in with some data. > > > > > > Thanks > > > Eno > > > > > > On Mon, Aug 20, 2018 at 7:29 AM, Becket Qin <becket....@gmail.com> > > wrote: > > > > > >> Hi Lucas, > > >> > > >> In KIP-103, we introduced a convention to define and look up the > > listeners. > > >> So it would be good if the later KIPs can follow the same convention. > > >> > > >> From what I understand, the advertised.listeners is actually designed > > for > > >> our purpose, i.e. providing a list of listeners that can be used in > > >> different cases. In KIP-103 it was used to separate internal traffic > > from > > >> the external traffic. It is not just for the user traffic or data > > >> only. So adding > > >> a controller listener is not repurposing the config. Also, ZK > structure > > is > > >> only visible to brokers, the clients will still only see the listeners > > they > > >> are seeing today. > > >> > > >> For this KIP, we are essentially trying to separate the controller > > traffic > > >> from the inter-broker data traffic. So adding a new > > >> listener.name.for.controller config seems reasonable. The behavior > would > > >> be: > > >> 1. If the listener.name.for.controller is set, the broker-controller > > >> communication will go through that listener. > > >> 2. Otherwise, the controller traffic falls back to use > > >> inter.broker.listener.name or inter.broker.security.protocol, which > is > > the > > >> current behavior. > > >> > > >> Regarding updating the security protocol with one line change v.s > > two-lines > > >> change, I am a little confused, can you elaborate? > > >> > > >> Regarding the possibility of hurry and misreading. It is the system > > admin's > > >> responsibility to configure the right listener to ensure that > different > > >> kinds of traffic are using the correct endpoints. So I think it is > > better > > >> that we always follow the same of convention instead of doing it in > > >> different ways. > > >> > > >> Thanks, > > >> > > >> Jiangjie (Becket) Qin > > >> > > >> > > >> > > >> On Fri, Aug 17, 2018 at 4:34 AM, Lucas Wang <lucasatu...@gmail.com> > > wrote: > > >> > > >>> Thanks for the review, Becket. > > >>> > > >>> (1) After comparing the two approaches, I still feel the current > > writeup > > >> is > > >>> a little better. > > >>> a. The current writeup asks for an explicit endpoint while reusing > the > > >>> existing "inter.broker.listener.name" with the exactly same > semantic, > > >>> and your proposed change asks for a new listener name for controller > > >> while > > >>> reusing the existing "advertised.listeners" config with a slight > > semantic > > >>> change since a new controller endpoint needs to be added to it. > > >>> Hence conceptually the current writeup requires one config change > > instead > > >>> of two. > > >>> Also with one listener name, e.g. INTERNAL, for inter broker traffic, > > >>> instead of two, e.g. "INTERNAL" and "CONTROLLER", > > >>> if an operator decides to switch from PLAINTEXT to SSL for internal > > >>> traffic, chances are that she wants to upgrade > > >>> both controller connections and data connections, she only needs to > > >> update > > >>> one line in > > >>> the "listener.security.protocol.map" config, and avoids possible > > >> mistakes. > > >>> > > >>> > > >>> b. When this KIP is picked up by an operator who is in a hurry > without > > >>> reading the docs, if she sees a > > >>> new listener name for controller is required, and chances are there > is > > >>> already a list of listeners, > > >>> it's possible for her to simply choose an existing listener name, > > without > > >>> explicitly creating > > >>> the new CONTROLLER listener and endpoints. If this is done, Kafka > will > > be > > >>> run with the existing > > >>> behavior, defeating the purpose of this KIP. > > >>> In comparison, if she sees a separate endpoint is being asked, I feel > > >> it's > > >>> unlikely for her to > > >>> copy and paste an existing endpoint. > > >>> > > >>> Please let me know your comments. > > >>> > > >>> (2) Good catch, it's a typo, and it's been fixed. > > >>> > > >>> Thanks, > > >>> Lucas > > >>> > > >> > > > > >
