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