+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
