Hi Boyang,
Sorry for being late on this. I'm generally in favor of the KIP, but it
seems like it has two missing things:
1. It doesn't cover how this new channel will be configured. That's a
critical part of having a KIP that comprises all that is needed to merge
the relevant PR.
2. It doesn't state the compatibility impact of enforcing the create topics
policy on auto topic creation. Furthermore, it doesn't explain the error
code that will be returned if the topic policy fails. Older clients won't
expect a PolicyValidationError during metadata requests, so we need to
discuss the approach there and what will happen for newer clients.
Ismael
On Thu, Jun 18, 2020 at 7:57 AM Boyang Chen <reluctanthero...@gmail.com>
wrote:
> Thanks everyone for the vote! We already got 3 binding votes (Colin,
> Guozhang, Rajini) and 2 non-binding votes (Jose, David). The KIP is now
> approved.
>
> Best,
> Boyang
>
> On Thu, Jun 18, 2020 at 1:43 AM Rajini Sivaram <rajinisiva...@gmail.com>
> wrote:
>
> > +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
> > > >
> > >
> >
>
