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