Thanks Jun, I've updated the KIP with the new names. Hi Joel, Becket, Dong, Ismael, Since you've reviewed this KIP in the past, can you please review it again? Thanks a lot!
Lucas On Mon, Oct 8, 2018 at 6:10 PM Jun Rao <j...@confluent.io> wrote: > Hi, Lucas, > > Yes, the new names sound good to me. > > Thanks, > > Jun > > On Fri, Oct 5, 2018 at 1:12 PM, Lucas Wang <lucasatu...@gmail.com> wrote: > > > Thanks for the suggestion, Ismael. I like it. > > > > Jun, > > I'm excited to get the +1, thanks a lot! > > Meanwhile what do you feel about renaming the metrics and config to > > > > ControlPlaneRequestQueueSize > > > > ControlPlaneNetworkProcessorIdlePercent > > > > ControlPlaneRequestHandlerIdlePercent > > > > control.plane.listener.name > > > > ? > > > > > > Thanks, > > > > Lucas > > > > On Thu, Oct 4, 2018 at 11:38 AM Ismael Juma <isma...@gmail.com> wrote: > > > > > Have we considered control plane if we think control by itself is > > > ambiguous? I agree with the original concern that "controller" may be > > > confusing for something that affects all brokers. > > > > > > Ismael > > > > > > > > > On 4 Oct 2018 11:08 am, "Lucas Wang" <lucasatu...@gmail.com> wrote: > > > > > > Thanks Jun. I've changed the KIP with the suggested 2 step upgrade. > > > Please take a look again when you have time. > > > > > > Regards, > > > Lucas > > > > > > > > > On Thu, Oct 4, 2018 at 10:06 AM Jun Rao <j...@confluent.io> wrote: > > > > > > > Hi, Lucas, > > > > > > > > 200. That's a valid concern. So, we can probably just keep the > current > > > > name. > > > > > > > > 201. I am thinking that you would upgrade in the same way as changing > > > > inter.broker.listener.name. This requires 2 rounds of rolling > restart. > > > In > > > > the first round, we add the controller endpoint to the listeners w/o > > > > setting controller.listener.name. In the second round, every broker > > sets > > > > controller.listener.name. At that point, the controller listener is > > > ready > > > > in every broker. > > > > > > > > Thanks, > > > > > > > > Jun > > > > > > > > On Tue, Oct 2, 2018 at 10:38 AM, Lucas Wang <lucasatu...@gmail.com> > > > wrote: > > > > > > > > > Thanks for the further comments, Jun. > > > > > > > > > > 200. Currently in the code base, we have the term of "ControlBatch" > > > > related > > > > > to > > > > > idempotent/transactional producing. Do you think it's a concern for > > > > reusing > > > > > the term "control"? > > > > > > > > > > 201. It's not clear to me how it would work by following the same > > > > strategy > > > > > for "controller.listener.name". > > > > > Say the new controller has its "controller.listener.name" set to > the > > > > value > > > > > "CONTROLLER", and broker 1 > > > > > has picked up this KIP by announcing > > > > > "endpoints": [ > > > > > "CONTROLLER://broker1.example.com:9091", > > > > > "INTERNAL://broker1.example.com:9092", > > > > > "EXTERNAL://host1.example.com:9093" > > > > > ], > > > > > > > > > > while broker2 has not picked up the change, and is announcing > > > > > "endpoints": [ > > > > > "INTERNAL://broker2.example.com:9092", > > > > > "EXTERNAL://host2.example.com:9093" > > > > > ], > > > > > to support both broker 1 for the new behavior and broker 2 for the > > old > > > > > behavior, it seems the controller must > > > > > check their published endpoints. Am I missing something? > > > > > > > > > > Thanks! > > > > > Lucas > > > > > > > > > > On Mon, Oct 1, 2018 at 6:29 PM Jun Rao <j...@confluent.io> wrote: > > > > > > > > > > > Hi, Lucas, > > > > > > > > > > > > Sorry for the delay. The updated wiki looks good to me overall. > > Just > > > a > > > > > > couple more minor comments. > > > > > > > > > > > > 200. > > > kafka.network:name=ControllerRequestQueueSize,type=RequestChannel: > > > > > The > > > > > > name ControllerRequestQueueSize gives the impression that it's > only > > > for > > > > > the > > > > > > controller broker. Perhaps we can just rename all metrics and > > configs > > > > > from > > > > > > controller to control. This indicates that the threads and the > > queues > > > > are > > > > > > for the control requests (as oppose to data requests). > > > > > > > > > > > > 201. "<new controller, old broker>: In this scenario, the > > controller > > > > will > > > > > > have the "controller.listener.name" config set to a value like > > > > > > "CONTROLLER", however the broker's exposed endpoints do not have > an > > > > entry > > > > > > corresponding to the new listener name. Hence the controller > should > > > > > > preserve the existing behavior by determining the endpoint using > > > > > > *inter-broker-listener-name *value. The end result should be the > > same > > > > > > behavior as today." Currently, the controller makes connections > > based > > > > on > > > > > > its local inter.broker.listener.name config without checking the > > > > target > > > > > > broker's ZK registration. For consistency, perhaps we can just > > follow > > > > the > > > > > > same strategy for controller.listener.name. This existing > behavior > > > > seems > > > > > > simpler to understand and has the benefit of catching > inconsistent > > > > > configs > > > > > > across brokers. > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Jun > > > > > > > > > > > > On Mon, Oct 1, 2018 at 8:43 AM, Lucas Wang < > lucasatu...@gmail.com> > > > > > wrote: > > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > > > Sorry to bother you again. Can you please take a look at the > wiki > > > > again > > > > > > > when you have time? > > > > > > > > > > > > > > Thanks a lot! > > > > > > > Lucas > > > > > > > > > > > > > > On Wed, Sep 19, 2018 at 3:57 PM Lucas Wang < > > lucasatu...@gmail.com> > > > > > > wrote: > > > > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > > > > > Thanks a lot for the detailed explanation. > > > > > > > > I've restored the wiki to a previous version that does not > > > require > > > > > > config > > > > > > > > changes, > > > > > > > > and keeps the current behavior with the proposed changes > turned > > > off > > > > > by > > > > > > > > default. > > > > > > > > I'd appreciate it if you can review it again. > > > > > > > > > > > > > > > > Thanks! > > > > > > > > Lucas > > > > > > > > > > > > > > > > On Tue, Sep 18, 2018 at 1:48 PM Jun Rao <j...@confluent.io> > > > wrote: > > > > > > > > > > > > > > > >> Hi, Lucas, > > > > > > > >> > > > > > > > >> When upgrading to a minor release, I think the expectation > is > > > > that a > > > > > > > user > > > > > > > >> wouldn't need to make any config changes, other than the > usual > > > > > > > >> inter.broker.protocol. If we require other config changes > > during > > > > an > > > > > > > >> upgrade, then it's probably better to do that in a major > > > release. > > > > > > > >> > > > > > > > >> Regarding your proposal, I think removing > host/advertised_host > > > in > > > > > > favor > > > > > > > of > > > > > > > >> listeners:advertised_listeners seems useful regardless of > this > > > > KIP. > > > > > > > >> However, that can probably wait until a major release. > > > > > > > >> > > > > > > > >> As for the controller listener, I am not sure if one has to > > set > > > > it. > > > > > To > > > > > > > >> make > > > > > > > >> a cluster healthy, one sort of have to make sure that the > > > request > > > > > > queue > > > > > > > is > > > > > > > >> never full and no request will be sitting in the request > queue > > > for > > > > > > long. > > > > > > > >> If > > > > > > > >> one does that, setting the controller listener may not be > > > > necessary. > > > > > > On > > > > > > > >> the > > > > > > > >> flip side, even if one sets the controller listener, but the > > > > request > > > > > > > queue > > > > > > > >> and the request time for the data part are still high, the > > > cluster > > > > > may > > > > > > > >> still not be healthy. Given that we have already started the > > 2.1 > > > > > > release > > > > > > > >> planning, perhaps we can start with not requiring the > > controller > > > > > > > listener. > > > > > > > >> If this is indeed something that everyone wants to set, we > can > > > > make > > > > > > it a > > > > > > > >> required config in a major release. > > > > > > > >> > > > > > > > >> Thanks, > > > > > > > >> > > > > > > > >> Jun > > > > > > > >> > > > > > > > >> On Tue, Sep 11, 2018 at 3:46 PM, Lucas Wang < > > > > lucasatu...@gmail.com> > > > > > > > >> wrote: > > > > > > > >> > > > > > > > >> > @Jun Rao <j...@confluent.io> > > > > > > > >> > > > > > > > > >> > I made the recent config changes after thinking about the > > > > default > > > > > > > >> behavior > > > > > > > >> > for adopting this KIP. > > > > > > > >> > I think there are basically two options: > > > > > > > >> > 1. By default, the behavior proposed in this KIP is turned > > > off, > > > > > and > > > > > > > >> > operators can turn it > > > > > > > >> > on by adding the "controller.listener.name" config and > > > entries > > > > in > > > > > > the > > > > > > > >> > "listeners" and "advertised.listeners" list. > > > > > > > >> > If no "controller.listener.name" is added, it'll be the > > *same > > > > as* > > > > > > > the " > > > > > > > >> > inter.broker.listener.name", > > > > > > > >> > and the proposed feature is effectively turned off. > > > > > > > >> > This has been the assumption in the KIP writeup before. > > > > > > > >> > > > > > > > > >> > 2. By default, the behavior proposed in this KIP is turned > > on, > > > > and > > > > > > > >> > operators are forced to > > > > > > > >> > recognize the proposed change if their "listeners" config > is > > > set > > > > > > (this > > > > > > > >> is > > > > > > > >> > most likely in production environments), > > > > > > > >> > by allocating a new port for controller connections, and > > > adding > > > > a > > > > > > new > > > > > > > >> > endpoint to the "listeners" config. > > > > > > > >> > For cases where "listeners" is not set explicitly, > > > > > > > >> > there needs to be a default value for it that includes the > > > > > > controller > > > > > > > >> > listener name, > > > > > > > >> > e.g. "PLAINTEXT://:9092,CONTROLLER://:9091" > > > > > > > >> > > > > > > > > >> > I chose to go with option 2 since as author of this KIP, > > > > > > > >> > I naturally think in the long run, it's worth the effort > to > > > > adopt > > > > > > this > > > > > > > >> > feature, > > > > > > > >> > in order to prevent issues under circumstances listed in > the > > > > > > > motivation > > > > > > > >> > section. > > > > > > > >> > > > > > > > > >> > 100, following the argument above, I want to enforce the > > > > > separation > > > > > > > >> > between controller > > > > > > > >> > and data plane requests. Hence the " > > controller.listener.name" > > > > > > should > > > > > > > >> > never be the same > > > > > > > >> > as the "inter.broker.listener.name", which is intended > for > > > data > > > > > > plane > > > > > > > >> > requests. > > > > > > > >> > > > > > > > > >> > 101, the default value for "listeners" will be > > > > > > > >> > "PLAINTEXT://:9092,CONTROLLER://:9091", > > > > > > > >> > making values of "host", and "port" not being used under > any > > > > > config > > > > > > > >> > settings. > > > > > > > >> > And the default value for "advertised.listeners" will be > > > derived > > > > > > from > > > > > > > >> > "listeners", > > > > > > > >> > making the values of "advertised.host", and > > "advertised.port" > > > > not > > > > > > > being > > > > > > > >> > used any more. > > > > > > > >> > > > > > > > > >> > 102, for upgrading, a single broker that has "listeners" > > > and/or > > > > > > > >> > "advertised.listeners" set, > > > > > > > >> > must add a new endpoint for the CONTROLLER listener name, > or > > > end > > > > > up > > > > > > > >> using > > > > > > > >> > the default listeners "PLAINTEXT://:9092,CONTROLLER: > > //:9091". > > > > > > > >> > During rolling upgrade, in cases of <old controller> + > <new > > > > > broker> > > > > > > or > > > > > > > >> > <new controller> + <old broker> > > > > > > > >> > we still need to fall back to the current behavior. > However > > > > after > > > > > > the > > > > > > > >> > rolling upgrade is done, > > > > > > > >> > it is guaranteed that the controller plane and data plane > > are > > > > > > > separated, > > > > > > > >> > given > > > > > > > >> > the "controller.listener.name" must be different from " > > > > > > > >> > inter.broker.listener.name". > > > > > > > >> > > > > > > > > >> > @Ismael Juma <ism...@juma.me.uk> > > > > > > > >> > Thanks for pointing that out. I did not know that. > > > > > > > >> > However my question is if the argument above makes sense, > > and > > > my > > > > > > code > > > > > > > >> > change > > > > > > > >> > causes the configs "host", "port", "advertised.host", > > > > > > > "advertised.port" > > > > > > > >> to > > > > > > > >> > be not used under any circumstance, > > > > > > > >> > then it's no different from removing them. > > > > > > > >> > Anyway if there is still a concern about removing them, is > > > > there a > > > > > > new > > > > > > > >> > major new version > > > > > > > >> > now or in the future where I can remove them? > > > > > > > >> > > > > > > > > >> > Thanks! > > > > > > > >> > Lucas > > > > > > > >> > > > > > > > > >> > On Mon, Sep 10, 2018 at 1:30 PM Ismael Juma < > > > ism...@juma.me.uk> > > > > > > > wrote: > > > > > > > >> > > > > > > > > >> >> To be clear, we can only remove configs in major new > > > versions. > > > > > > > >> Otherwise, > > > > > > > >> >> we can only deprecate them. > > > > > > > >> >> > > > > > > > >> >> Ismael > > > > > > > >> >> > > > > > > > >> >> On Mon, Sep 10, 2018 at 10:47 AM Jun Rao < > j...@confluent.io > > > > > > > > wrote: > > > > > > > >> >> > > > > > > > >> >> > Hi, Lucas, > > > > > > > >> >> > > > > > > > > >> >> > For the network idlePct, your understanding is correct. > > > > > > Currently, > > > > > > > >> >> > networkIdlePct metric is calculated as the average of > (1 > > - > > > > > > > io-ratio) > > > > > > > >> in > > > > > > > >> >> the > > > > > > > >> >> > selector of all network threads. > > > > > > > >> >> > > > > > > > > >> >> > The metrics part looks good to me in the updated KIP. > > > > > > > >> >> > > > > > > > > >> >> > I am not still not quite sure about the configs. > > > > > > > >> >> > > > > > > > > >> >> > 100. "Whenever the "controller.listener.name" is set, > > upon > > > > > > broker > > > > > > > >> >> startup, > > > > > > > >> >> > we will validate its value and make sure it's different > > > from > > > > > the > > > > > > > >> >> > *inter-broker-listener-name *value." Does that mean > that > > > > > > > >> >> > controller.listener.name has to be different from > > > > > > > >> >> > inter.broker.listener.name? > > > > > > > >> >> > That seems limiting. > > > > > > > >> >> > > > > > > > > >> >> > 101. The KIP says that advertised.listeners and > listeners > > > > will > > > > > > now > > > > > > > >> have > > > > > > > >> >> a > > > > > > > >> >> > different default value including controller. Could you > > > > > document > > > > > > > what > > > > > > > >> >> the > > > > > > > >> >> > default value looks like? > > > > > > > >> >> > > > > > > > > >> >> > 102. About removing the the following configs. How does > > > that > > > > > > affect > > > > > > > >> the > > > > > > > >> >> > upgrade path? Do we now expect a user to add a new > config > > > > when > > > > > > > >> upgrading > > > > > > > >> >> > from an old version to a new one? > > > > > > > >> >> > host > > > > > > > >> >> > port > > > > > > > >> >> > advertised.host > > > > > > > >> >> > advertised.port > > > > > > > >> >> > > > > > > > > >> >> > Thanks, > > > > > > > >> >> > > > > > > > > >> >> > Jun > > > > > > > >> >> > > > > > > > > >> >> > > > > > > > > >> >> > On Thu, Sep 6, 2018 at 5:14 PM, Lucas Wang < > > > > > > lucasatu...@gmail.com> > > > > > > > >> >> wrote: > > > > > > > >> >> > > > > > > > > >> >> > > @Jun Rao <j...@confluent.io> > > > > > > > >> >> > > > > > > > > > >> >> > > One clarification, currently on the selector level, > we > > > have > > > > > the > > > > > > > >> >> > > io-wait-ratio metric. > > > > > > > >> >> > > For the new controller *network* thread, we can use > it > > > > > directly > > > > > > > for > > > > > > > >> >> > > IdlePct, instead of using 1- io-ratio, > > > > > > > >> >> > > so that the logic is similar to the current average > > > IdlePct > > > > > for > > > > > > > >> >> network > > > > > > > >> >> > > threads. Is that correct? > > > > > > > >> >> > > > > > > > > > >> >> > > I've revised the KIP by adding two new metrics for > > > > measuring > > > > > > the > > > > > > > >> >> IdlePct > > > > > > > >> >> > > for the two additional threads. > > > > > > > >> >> > > Please take a look again. Thanks! > > > > > > > >> >> > > > > > > > > > >> >> > > Lucas > > > > > > > >> >> > > > > > > > > > >> >> > > > > > > > > > >> >> > > > > > > > > > >> >> > > > > > > > > > >> >> > > > > > > > > > >> >> > > On Wed, Sep 5, 2018 at 5:01 PM Jun Rao < > > j...@confluent.io > > > > > > > > > > wrote: > > > > > > > >> >> > > > > > > > > > >> >> > > > Hi, Lucas, > > > > > > > >> >> > > > > > > > > > > >> >> > > > Thanks for the updated KIP. > > > > > > > >> >> > > > > > > > > > > >> >> > > > For monitoring the network thread utilization for > the > > > > > control > > > > > > > >> >> plane, we > > > > > > > >> >> > > > already have the metric io-ratio at the selector > > level > > > > > > (idlePct > > > > > > > >> is > > > > > > > >> >> 1 - > > > > > > > >> >> > > > io-ratio). So, we just need to give that selector a > > > > > > meaningful > > > > > > > >> name. > > > > > > > >> >> > > > > > > > > > > >> >> > > > For monitoring the io thread utilization for the > > > control > > > > > > plane, > > > > > > > >> it's > > > > > > > >> >> > > > probably useful to have a separate metric for that. > > The > > > > > > > >> controller > > > > > > > >> >> > > request > > > > > > > >> >> > > > queue size may not reflect the history in a window. > > > > > > > >> >> > > > > > > > > > > >> >> > > > Jun > > > > > > > >> >> > > > > > > > > > > >> >> > > > On Wed, Sep 5, 2018 at 3:38 PM, Lucas Wang < > > > > > > > >> lucasatu...@gmail.com> > > > > > > > >> >> > > wrote: > > > > > > > >> >> > > > > > > > > > > >> >> > > > > Thanks Jun for your quick response. It looks > like I > > > > > forgot > > > > > > to > > > > > > > >> >> click > > > > > > > >> >> > the > > > > > > > >> >> > > > > "Update" button, :) > > > > > > > >> >> > > > > It's updated now. > > > > > > > >> >> > > > > > > > > > > > >> >> > > > > Regarding the idle ratio metrics for the > additional > > > > > > threads, > > > > > > > I > > > > > > > >> >> > > discussed > > > > > > > >> >> > > > > with Joel, > > > > > > > >> >> > > > > and think they are not as useful, and I added our > > > > > reasoning > > > > > > > in > > > > > > > >> the > > > > > > > >> >> > last > > > > > > > >> >> > > > > paragraph of the > > > > > > > >> >> > > > > "How are controller requests handled over the > > > dedicated > > > > > > > >> >> connections?" > > > > > > > >> >> > > > > section. > > > > > > > >> >> > > > > On the other hand, we don't strongly oppose > adding > > > them > > > > > if > > > > > > > you > > > > > > > >> >> think > > > > > > > >> >> > > they > > > > > > > >> >> > > > > are necessary. > > > > > > > >> >> > > > > > > > > > > > >> >> > > > > Thanks, > > > > > > > >> >> > > > > Lucas > > > > > > > >> >> > > > > > > > > > > > >> >> > > > > > > > > > > > >> >> > > > > On Wed, Sep 5, 2018 at 3:12 PM Jun Rao < > > > > j...@confluent.io > > > > > > > > > > > > > >> wrote: > > > > > > > >> >> > > > > > > > > > > > >> >> > > > > > Hi, Lucas, > > > > > > > >> >> > > > > > > > > > > > > >> >> > > > > > Thanks for the reply. Have you actually updated > > the > > > > > KIP? > > > > > > > The > > > > > > > >> >> wiki > > > > > > > >> >> > > says > > > > > > > >> >> > > > > that > > > > > > > >> >> > > > > > it's last updated on Aug. 22. and some of the > > > changes > > > > > > that > > > > > > > >> you > > > > > > > >> >> > > > mentioned > > > > > > > >> >> > > > > > (#1 and #3) are not there. > > > > > > > >> >> > > > > > > > > > > > > >> >> > > > > > Also, regarding Joel's comment on > network/request > > > > idle > > > > > > > ratio > > > > > > > >> >> > metrics, > > > > > > > >> >> > > > > could > > > > > > > >> >> > > > > > you comment on whether they include the new > > > > controller > > > > > > > >> >> listener? If > > > > > > > >> >> > > > not, > > > > > > > >> >> > > > > do > > > > > > > >> >> > > > > > we need additional metrics to measure the > > > utilization > > > > > of > > > > > > > the > > > > > > > >> io > > > > > > > >> >> > > thread > > > > > > > >> >> > > > > for > > > > > > > >> >> > > > > > the control plane? > > > > > > > >> >> > > > > > > > > > > > > >> >> > > > > > Jun > > > > > > > >> >> > > > > > > > > > > > > >> >> > > > > > On Mon, Aug 27, 2018 at 6:26 PM, Lucas Wang < > > > > > > > >> >> lucasatu...@gmail.com > > > > > > > >> >> > > > > > > > > > >> >> > > > > wrote: > > > > > > > >> >> > > > > > > > > > > > > >> >> > > > > > > Thanks for the comments, Jun. > > > > > > > >> >> > > > > > > > > > > > > > >> >> > > > > > > 1. I think the answer should be no, since > the " > > > > > > > >> >> > > > > > inter.broker.listener.name" > > > > > > > >> >> > > > > > > are also used > > > > > > > >> >> > > > > > > for replication traffic, and merging these > two > > > > types > > > > > of > > > > > > > >> >> request > > > > > > > >> >> > to > > > > > > > >> >> > > > the > > > > > > > >> >> > > > > > > single threaded tunnel > > > > > > > >> >> > > > > > > would defeat the purpose of this KIP and also > > > hurt > > > > > > > >> replication > > > > > > > >> >> > > > > > throughput. > > > > > > > >> >> > > > > > > So I think that means > > > > > > > >> >> > > > > > > we should validate to make sure when the new > > > config > > > > > is > > > > > > > set, > > > > > > > >> >> it's > > > > > > > >> >> > > > > > different > > > > > > > >> >> > > > > > > from "inter.broker.listener.name" > > > > > > > >> >> > > > > > > or "security.inter.broker.protocol", > > whichever is > > > > > set. > > > > > > > >> >> > > > > > > > > > > > > > >> >> > > > > > > 2. Normally all broker configs in a given > > cluster > > > > are > > > > > > > >> changed > > > > > > > >> >> at > > > > > > > >> >> > > the > > > > > > > >> >> > > > > same > > > > > > > >> >> > > > > > > time. If there is a typo in the > > > > > > > >> >> > > > > > > controller.listener.name and it's not > > available > > > in > > > > > the > > > > > > > >> >> endpoints > > > > > > > >> >> > > > list, > > > > > > > >> >> > > > > > we > > > > > > > >> >> > > > > > > could catch it, give an error > > > > > > > >> >> > > > > > > and block restart of the first broker in the > > > > cluster. > > > > > > > With > > > > > > > >> >> that, > > > > > > > >> >> > we > > > > > > > >> >> > > > > could > > > > > > > >> >> > > > > > > keep the current behavior > > > > > > > >> >> > > > > > > in the KIP write up that falls back to > > > > > > > >> >> > "inter.broker.listener.nam" > > > > > > > >> >> > > > when > > > > > > > >> >> > > > > > the > > > > > > > >> >> > > > > > > "controller.listener.name" > > > > > > > >> >> > > > > > > is not found during the migration phase of > this > > > > KIP. > > > > > > > >> Thoughts? > > > > > > > >> >> > > > > > > > > > > > > > >> >> > > > > > > 3. That makes sense, and I've changed it. > > > > > > > >> >> > > > > > > > > > > > > > >> >> > > > > > > Thanks, > > > > > > > >> >> > > > > > > Lucas > > > > > > > >> >> > > > > > > > > > > > > > >> >> > > > > > > On Thu, Aug 23, 2018 at 3:46 PM Jun Rao < > > > > > > > j...@confluent.io> > > > > > > > >> >> > wrote: > > > > > > > >> >> > > > > > > > > > > > > > >> >> > > > > > > > Hi, Lucas, > > > > > > > >> >> > > > > > > > > > > > > > > >> >> > > > > > > > Sorry for the delay. The new proposal looks > > > good > > > > to > > > > > > me > > > > > > > >> >> > overall. A > > > > > > > >> >> > > > few > > > > > > > >> >> > > > > > > minor > > > > > > > >> >> > > > > > > > comments below. > > > > > > > >> >> > > > > > > > > > > > > > > >> >> > > > > > > > 1. It's possible that > > > > listener.name.for.controller > > > > > is > > > > > > > >> set, > > > > > > > >> >> but > > > > > > > >> >> > > set > > > > > > > >> >> > > > to > > > > > > > >> >> > > > > > the > > > > > > > >> >> > > > > > > > same value as inter.broker.listener.name. > In > > > > that > > > > > > > case, > > > > > > > >> >> should > > > > > > > >> >> > > we > > > > > > > >> >> > > > > > have a > > > > > > > >> >> > > > > > > > single network thread and the request > > handling > > > > > thread > > > > > > > for > > > > > > > >> >> that > > > > > > > >> >> > > > > > listener? > > > > > > > >> >> > > > > > > > > > > > > > > >> >> > > > > > > > 2. Currently, the controller always picks > the > > > > > > listener > > > > > > > >> >> > specified > > > > > > > >> >> > > by > > > > > > > >> >> > > > > > > > inter.broker.listener.name even if the > > > listener > > > > > name > > > > > > > is > > > > > > > >> not > > > > > > > >> >> > > > present > > > > > > > >> >> > > > > in > > > > > > > >> >> > > > > > > the > > > > > > > >> >> > > > > > > > receiving broker. This KIP proposes a > > slightly > > > > > > > different > > > > > > > >> >> > approach > > > > > > > >> >> > > > for > > > > > > > >> >> > > > > > > > picking listener.name.for.controller only > > when > > > > the > > > > > > > >> receiving > > > > > > > >> >> > end > > > > > > > >> >> > > > has > > > > > > > >> >> > > > > > the > > > > > > > >> >> > > > > > > > listener and switches > > > > listener.name.for.controller > > > > > > > >> >> otherwise. > > > > > > > >> >> > > There > > > > > > > >> >> > > > > are > > > > > > > >> >> > > > > > > > some tradeoffs between the two approaches. > To > > > > > change > > > > > > > the > > > > > > > >> >> inter > > > > > > > >> >> > > > broker > > > > > > > >> >> > > > > > > > listener, the former requires 2 steps: (1) > > > adding > > > > > the > > > > > > > new > > > > > > > >> >> > > listener > > > > > > > >> >> > > > to > > > > > > > >> >> > > > > > > > listener list in every broker and (2) > > changing > > > > > > > >> >> > > > > > > > listener.name.for.controller. > > > > > > > >> >> > > > > > > > The latter can do both changes in 1 step. > On > > > the > > > > > > hand, > > > > > > > if > > > > > > > >> >> > > > > > > > listener.name.for.controller > > > > > > > >> >> > > > > > > > is mis-configured, the former will report > an > > > > error > > > > > > and > > > > > > > >> the > > > > > > > >> >> > latter > > > > > > > >> >> > > > > will > > > > > > > >> >> > > > > > > hide > > > > > > > >> >> > > > > > > > it (so the user may not know the > > > > misconfiguration). > > > > > > It > > > > > > > >> seems > > > > > > > >> >> > that > > > > > > > >> >> > > > we > > > > > > > >> >> > > > > > > should > > > > > > > >> >> > > > > > > > pick one approach to handle both > > > > > > > >> >> listener.name.for.controller > > > > > > > >> >> > and > > > > > > > >> >> > > > > > > > inter.broker.listener.name consistently. > To > > > me, > > > > > the > > > > > > > >> former > > > > > > > >> >> > seems > > > > > > > >> >> > > > > > > slightly > > > > > > > >> >> > > > > > > > better. > > > > > > > >> >> > > > > > > > > > > > > > > >> >> > > > > > > > 3. To be consistent with the existing > naming, > > > > > should > > > > > > > >> >> > > > > > > > listener.name.for.controller > > > > > > > >> >> > > > > > > > be controller.listener.name? > > > > > > > >> >> > > > > > > > > > > > > > > >> >> > > > > > > > Thanks, > > > > > > > >> >> > > > > > > > > > > > > > > >> >> > > > > > > > Jun > > > > > > > >> >> > > > > > > > > > > > > > > >> >> > > > > > > > > > > > > > > >> >> > > > > > > > On Thu, Aug 9, 2018 at 3:21 PM, Lucas Wang > < > > > > > > > >> >> > > lucasatu...@gmail.com> > > > > > > > >> >> > > > > > > wrote: > > > > > > > >> >> > > > > > > > > > > > > > > >> >> > > > > > > > > Hi Jun and Joel, > > > > > > > >> >> > > > > > > > > > > > > > > > >> >> > > > > > > > > The KIP writeup has changed by quite a > bit > > > > since > > > > > > your > > > > > > > >> +1. > > > > > > > >> >> > > > > > > > > Can you please take another review? > Thanks > > a > > > > lot > > > > > > for > > > > > > > >> your > > > > > > > >> >> > time! > > > > > > > >> >> > > > > > > > > > > > > > > > >> >> > > > > > > > > Lucas > > > > > > > >> >> > > > > > > > > > > > > > > > >> >> > > > > > > > > On Tue, Jul 17, 2018 at 10:33 AM, Joel > > Koshy > > > < > > > > > > > >> >> > > > jjkosh...@gmail.com> > > > > > > > >> >> > > > > > > > wrote: > > > > > > > >> >> > > > > > > > > > > > > > > > >> >> > > > > > > > > > +1 on the KIP. > > > > > > > >> >> > > > > > > > > > > > > > > > > >> >> > > > > > > > > > (I'm not sure we actually necessary to > > > > > introduce > > > > > > > the > > > > > > > >> >> > > condition > > > > > > > >> >> > > > > > > > variables > > > > > > > >> >> > > > > > > > > > for the concern that Jun raised, but > it's > > > an > > > > > > > >> >> implementation > > > > > > > >> >> > > > > detail > > > > > > > >> >> > > > > > > that > > > > > > > >> >> > > > > > > > > we > > > > > > > >> >> > > > > > > > > > can defer to a discussion in the PR.) > > > > > > > >> >> > > > > > > > > > > > > > > > > >> >> > > > > > > > > > On Sat, Jul 14, 2018 at 10:45 PM, Lucas > > > Wang > > > > < > > > > > > > >> >> > > > > > lucasatu...@gmail.com> > > > > > > > >> >> > > > > > > > > > wrote: > > > > > > > >> >> > > > > > > > > > > > > > > > > >> >> > > > > > > > > > > Hi Jun, > > > > > > > >> >> > > > > > > > > > > > > > > > > > >> >> > > > > > > > > > > I agree by using the conditional > > > variables, > > > > > > there > > > > > > > >> is > > > > > > > >> >> no > > > > > > > >> >> > > need > > > > > > > >> >> > > > to > > > > > > > >> >> > > > > > add > > > > > > > >> >> > > > > > > > > such > > > > > > > >> >> > > > > > > > > > a > > > > > > > >> >> > > > > > > > > > > new config. > > > > > > > >> >> > > > > > > > > > > Also thanks for approving this KIP. > > > > > > > >> >> > > > > > > > > > > > > > > > > > >> >> > > > > > > > > > > Lucas > > > > > > > >> >> > > > > > > > > > > > > > > > > > >> >> > > > > > > > > > > > > > > > > >> >> > > > > > > > > > > > > > > > >> >> > > > > > > > > > > > > > > >> >> > > > > > > > > > > > > > >> >> > > > > > > > > > > > > >> >> > > > > > > > > > > > >> >> > > > > > > > > > > >> >> > > > > > > > > > >> >> > > > > > > > > >> >> > > > > > > > >> > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >