Hi Eno, I fully agree with Becket here. If the motivation section makes sense, and we know we can get burnt by this problem, then the exact numbers (which vary case by case according to the config settings and traffic pattern) are no longer as important.
Thanks, Lucas On Tue, Aug 21, 2018 at 9:39 AM 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 > >>> > >> > >
