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