Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
MIME-Version: 1.0
Sender: ismaelj@gmail.com
In-Reply-To: <CAJDuW=CcUobJzy+mbZWNHiwX98paKP9cAf9ponmitzOFeO=-Jg@mail.gmail.com>
References: <CAD5tkZbT3+bS62hgP9QcY83k1B9K-QRQgbgTnLhE4kKPpgZSuw@mail.gmail.com>
 <CAFc58G9+9nKzz=w7bLDEHyqRgqve8JPyLr6Y_horcoTjaezfeg@mail.gmail.com>
 <1494017117.114782.967315616.7CF5E9B3@webmail.messagingengine.com>
 <CAD5tkZYteeF-OAhEiZXfjcKMz0PmaxDOPXQpk25yND2p_Of4OA@mail.gmail.com>
 <1494188947.3183807.968680040.22904EAD@webmail.messagingengine.com>
 <CAD5tkZa2B-0d2ax4cJjF5xRCi32mwumPtf_LVGKv9zC=n8qTnw@mail.gmail.com>
 <1494265650.810700.969747776.5BBA18F0@webmail.messagingengine.com> <CAJDuW=CcUobJzy+mbZWNHiwX98paKP9cAf9ponmitzOFeO=-Jg@mail.gmail.com>
From: Ismael Juma <ismael@juma.me.uk>
Date: Wed, 10 May 2017 23:09:05 +0100
Message-ID: <CAD5tkZZEF+Hw=m_F_EZeGK_V8F4Lx3AY1Gr_zFY2cY9jtz0PPg@mail.gmail.com>
Subject: Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs
To: dev@kafka.apache.org
Content-Type: multipart/alternative; boundary=001a11402662eb0fa2054f32ba2d
archived-at: Wed, 10 May 2017 22:09:53 -0000

--001a11402662eb0fa2054f32ba2d
Content-Type: text/plain; charset=UTF-8

Hi Jason,

Thanks for the feedback. I agree that it's useful and reasonably simple to
implement. So, I've updated the KIP.

Ismael

On Wed, May 10, 2017 at 10:48 PM, Jason Gustafson <jason@confluent.io>
wrote:

> Hey Ismael,
>
> Thanks for the KIP. The use of the Describe API might be a little limited
> if it always returns the full set of topics for the requested resource. I
> wonder if we can let the client provide a list of the configs they are
> interested in. Perhaps something like this:
>
> DescribeConfigs Request (Version: 0) => [resource [config_name]]
>   resource => resource_type resource_name
>     resource_type => INT8
>     resource_name => STRING
>   config_name => STRING
>
> This would reduce the overhead for use cases where the client only cares
> about a small subset of configs. A null array can then indicate the desire
> to fetch all configs. What do you think?
>
> Thanks,
> Jason
>
> On Mon, May 8, 2017 at 10:47 AM, Colin McCabe <cmccabe@apache.org> wrote:
>
> > On Sun, May 7, 2017, at 20:18, Ismael Juma wrote:
> > > Thanks for the feedback Colin. Comments inline.
> > >
> > > On Sun, May 7, 2017 at 9:29 PM, Colin McCabe <cmccabe@apache.org>
> wrote:
> > > >
> > > > Hmm.  What's the behavior if I try to list the configuration for a
> > topic
> > > > that doesn't exist?  It seems like in this case, the whole request
> has
> > > > to return an error, and nothing gets listed.  Shouldn't the protocol
> > > > should return an error code and message per element in the batch,
> > > > similar to how the other batch protocols work (CreateTopics,
> > > > DeleteTopics, etc. etc.)
> > > >
> > >
> > > CreateTopics and DeleteTopics are more like AlterConfigs and that has
> an
> > > error code per batch. For requests that mutate, this is essential
> because
> > > the operations are not transactional. I followed the same pattern as
> > > ListGroups, but that doesn't take filters, so MetadataRequest is a
> better
> > > example. For consistency with that, I added an error code per batch.
> > >
> > > > We also should think about the case where someone requests
> > > > configurations from multiple brokers.  Since there are multiple
> > requests
> > > > sent over the wire in this case, there is the chance for some of them
> > to
> > > > fail when others have succeeded.  So the API needs a way of returning
> > an
> > > > error per batch element.
> > > >
> > >
> > > Yeah, that's true. For the AdminClient side, I followed the same
> pattern
> > > as
> > > ListTopicsResponse, but it seems like DescribeTopicsResults is a better
> > > example. I named this request ListConfigs for consistency with
> ListAcls,
> > > but it looks like they really should be DescribeConfigs and
> DescribeAcls.
> > >
> > > > On the other hand, with configurations, each topic will have a single
> > > > configuration, never more and never less.  Each cluster will have a
> > > > single configuration-- never more and never less.  So having separate
> > > > Configuration resources doesn't seem to add any value, since they
> will
> > > > always map 1:1 to existing resources.
> > > >
> > >
> > > Good point. I thought about your proposal in more depth and I agree
> that
> > > it
> > > solves the issue in a nice way. I updated the KIP.
> >
> > Thanks, Ismael.
> >
> > >
> > > I guess my question is more conceptual-- if these things are all
> > > > resources, shouldn't we have a single type to model all of them?  We
> > > > might want to add configurations to other resources later on as well.
> > > >
> > >
> > > I've been thinking about how we could satisfy the 2 requirements of
> > > having
> > > a general resource type while making it clear which values are allowed
> > > for
> > > a given operation. I updated the KIP to use a shared resource type in
> the
> > > wire, renamed entity to resource, but kept a separate class and enum
> for
> > > ConfigResource and ConfigResource.Type. They map trivially to Resource
> > > and
> > > ResourceType.
> >
> > I still feel like it would be better to use the same type in the API.
> > I'm curious what others think here?
> >
> > >
> > > Also, I realised that I was a bit hasty when I changed Config to
> > > Collection<ConfigEntry> in the signature of a few methods. I think
> Config
> > > is the right type. It's a container for a Collection of ConfigEntry
> > > instances so that we can provide useful methods to work with the
> configs
> > > (e.g. exclude items with defaults, etc.).
> >
> > Good idea.  Should we add a Config#get(String) that can get the value of
> > a specific ConfigEntry?
> >
> > >
> > > Here's the full diff of the changes:
> > >
> > > https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?
> > pageId=68719318&selectedPageVersions=14&selectedPageVersions=11
> > >
> > > Given that we don't have much time until the KIP freeze and this is an
> > > important KIP to make the AdminClient truly useful, I will start the
> vote
> > > thread. That said, don't hesitate to provide additional feedback.
> >
> > +1.
> >
> > best,
> > Colin
> >
> > >
> > > Thanks.
> > > Ismael
> >
>

--001a11402662eb0fa2054f32ba2d--