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 > > > > > > > > > > > > > > >
