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