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: [DISCUSS] KIP-133: List and Alter Configs Admin APIs
Date Wed, 10 May 2017 22:09:05 GMT
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
> >
>

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