kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ismael Juma <ism...@juma.me.uk>
Subject Re: [VOTE] KIP-590: Redirect Zookeeper Mutation Protocols to The Controller
Date Thu, 18 Jun 2020 17:20:36 GMT
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 <reluctanthero104@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 <rajinisivaram@gmail.com>
> wrote:
>
> > +1 (binding)
> >
> > Thanks for the KIP, Boyang!
> >
> > Regards,
> >
> > Rajini
> >
> >
> >
> >
> > On Thu, Jun 18, 2020 at 8:15 AM David Jacot <djacot@confluent.io> wrote:
> >
> > > +1 (non-binding)
> > >
> > > Thanks for the KIP!
> > >
> > > On Thu, Jun 18, 2020 at 12:00 AM Jose Garcia Sancio <
> > jsancio@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 <wangguoz@gmail.com>
> > > wrote:
> > > > >
> > > > > Thanks for the KIP Boyang, +1 from me.
> > > > >
> > > > >
> > > > > Guozhang
> > > > >
> > > > > On Wed, Jun 17, 2020 at 1:40 PM Colin McCabe <cmccabe@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 <
> > cmccabe@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
<
> > > cmccabe@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
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message