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