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
>
