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 <[email protected]> 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
