+1 (binding)
Thanks for the KIP, Boyang!
Regards,
Rajini
On Thu, Jun 18, 2020 at 8:15 AM David Jacot <dja...@confluent.io> wrote:
> +1 (non-binding)
>
> Thanks for the KIP!
>
> On Thu, Jun 18, 2020 at 12:00 AM Jose Garcia Sancio <jsan...@confluent.io>
> wrote:
>
> > +1.
> >
> > Thanks for the KIP and looking forward to the improvement implementation.
> >
> > On Wed, Jun 17, 2020 at 2:24 PM Guozhang Wang <wangg...@gmail.com>
> wrote:
> > >
> > > Thanks for the KIP Boyang, +1 from me.
> > >
> > >
> > > Guozhang
> > >
> > > On Wed, Jun 17, 2020 at 1:40 PM Colin McCabe <cmcc...@apache.org>
> wrote:
> > >
> > > > Thanks, Boyang! +1 (binding)
> > > >
> > > > best,
> > > > Colin
> > > >
> > > > On Mon, Jun 15, 2020, at 12:59, Boyang Chen wrote:
> > > > > Thanks for more feedback Colin! I have addressed them in the KIP.
> > > > >
> > > > > Boyang
> > > > >
> > > > > On Mon, Jun 15, 2020 at 11:29 AM Colin McCabe <cmcc...@apache.org>
> > > > wrote:
> > > > >
> > > > > > On Fri, Jun 12, 2020, at 15:30, Boyang Chen wrote:
> > > > > > > Thanks Colin for the suggestions!
> > > > > > >
> > > > > > > On Fri, Jun 12, 2020 at 2:40 PM Colin McCabe <
> cmcc...@apache.org
> > >
> > > > wrote:
> > > > > > >
> > > > > > > > Hi Boyang,
> > > > > > > >
> > > > > > > > Thanks for the KIP! I think it's getting close.
> > > > > > > >
> > > > > > > > > For older requests that need redirection, forwarding
> > > > > > > > > broker will just use its own authorizer to verify the
> > > > principals.
> > > > > > When
> > > > > > > > the
> > > > > > > > > request looks good, it will just forward the request with
> > its
> > > > own
> > > > > > > > > credentials, no second validation needed
> > > > > > > >
> > > > > > > > Just to be clear, the controller will still validate the
> > request,
> > > > > > right?
> > > > > > > > But at that point the principal will be the broker principal.
> > It
> > > > > > would be
> > > > > > > > good to note that here.
> > > > > > > >
> > > > > > > > Sounds good, cleared in the KIP.
> > > > > > >
> > > > > > > > Internal CreateTopicsRequest Routing
> > > > > > > >
> > > > > > > > The forwarding broker is sending the request as the latest
> > version,
> > > > > > > > right? It would be good to add a note of this. This also
> > prevents
> > > > > > routing
> > > > > > > > loops since the latest version is not forwardable (another
> good
> > > > thing
> > > > > > to
> > > > > > > > add, I think...)
> > > > > > > >
> > > > > > > We are not bumping the CreateTopic RPC here, so it should be
> the
> > > > latest
> > > > > > > by default.
> > > > > > >
> > > > > >
> > > > > > Sorry, CreateTopics was a bad example here, since it already must
> > be
> > > > sent
> > > > > > to the controller. Oops.
> > > > > >
> > > > > > >
> > > > > > > And just to be clear, we are not "forwarding" but actually
> > > > > > > sending a CreateTopicRequest from the receiving broker to the
> > > > controller
> > > > > > > broker.
> > > > > > >
> > > > > >
> > > > > > Right. I think we agree on this point. But we do need a term to
> > > > describe
> > > > > > the broker which initially receives the user request and resends
> > it to
> > > > the
> > > > > > controller. Resending broker?
> > > > > >
> > > > > > And I do think it's important to note that the request we send to
> > the
> > > > > > controller can't be itself resent.
> > > > > >
> > > > > > >
> > > > > > > > As we discussed in the request routing section, to work with
> > an
> > > > older
> > > > > > > > > client, the first contacted broker need to act as a proxy
> to
> > > > > > redirect
> > > > > > > > the
> > > > > > > > > write request to the controller. To support the proxy of
> > > > requests,
> > > > > > we
> > > > > > > > need
> > > > > > > > > to build a channel for brokers to talk directly to the
> > > > controller.
> > > > > > This
> > > > > > > > > part of the design is internal change only and won’t block
> > the
> > > > KIP
> > > > > > > > > progress.
> > > > > > > >
> > > > > > > > I think it's good to note that we eventually want a separate
> > > > controller
> > > > > > > > endpoint in KIP-500. However, we don't need it to implement
> > > > KIP-590,
> > > > > > > > right? The other brokers could forward to the existing
> > internal
> > > > > > endpoint
> > > > > > > > for the controller. So maybe it's best to discuss the
> separate
> > > > > > endpoint in
> > > > > > > > "future work" rather than here.
> > > > > > > >
> > > > > > > > I moved the new endpoint part towards the future work and
> > > > addressed the
> > > > > > > > usage of controller internal endpoint for routing requests.
> > > > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > > >
> > > > > > > > > =============== Start Old Proposal ===============
> > > > > > > >
> > > > > > > > I'm glad the old proposal shows up here, but I think this is
> > too
> > > > much
> > > > > > > > detail. It would be better to just have a one or two
> paragraph
> > > > > > summary of
> > > > > > > > the main points. As it is, the old proposal takes up 40% of
> > the
> > > > doc
> > > > > > which
> > > > > > > > is pretty confusing for someone reading through. Let's also
> > not
> > > > forget
> > > > > > > > that someone can just read the old version by using the "page
> > > > history"
> > > > > > > > function on the wiki. So there's no need to keep that all
> > here.
> > > > > > > >
> > > > > > > > Make sense, removed.
> > > > > > >
> > > > > >
> > > > > > Thanks again.
> > > > > >
> > > > > > >
> > > > > > > { "name": "PrincipalName", "type": "string", "tag": 0,
> > > > > > "taggedVersions": "2+", "ignorable": true,
> > > > > > > "about": "Optional value of the principal name when the
> > request
> > > > is
> > > > > > redirected by a broker." },
> > > > > > >
> > > > > >
> > > > > > Maybe "InitialPrincipalName" would be better here? PrincipalName
> > is a
> > > > bit
> > > > > > confusing since the message already has a principal name, after
> > all...
> > > > > >
> > > > > > cheers,
> > > > > > Colin
> > > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > > -- Guozhang
> >
> >
> >
> > --
> > -Jose
> >
>
