Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Ismael Juma]

Hey Colin,

I added Config#get(String) to the KIP.

Ismael

On Mon, May 8, 2017 at 6:47 PM, Colin McCabe  wrote:
>
> Good idea.  Should we add a Config#get(String) that can get the value of
> a specific ConfigEntry?
>

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Ismael Juma]

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 
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  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 
> 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 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=14=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
> >
> > >
> 

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Jason Gustafson]

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  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  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 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=14=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
>

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Colin McCabe]

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  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 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=14=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

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Ismael Juma]

I meant, ListConfigs to DescribeConfigs. :)

On Mon, May 8, 2017 at 4:50 PM, Ismael Juma  wrote:

> Quick update, I renamed ListAcls to DescribeAcls (and related classes and
> methods) as that is more consistent with other protocols (like ListGroups
> and DescribeGroups).
>
> Ismael
>
> On Mon, May 8, 2017 at 2:17 PM, Ismael Juma  wrote:
>
>> Hi James,
>>
>> Yes, we will have a place where we perform validation independently of
>> the policy. This will be quite basic in the initial version, but we can
>> improve it over time. We will be able to do better than `ConfigCommand`
>> because we will have access to the broker topic configs. Without access to
>> the broker topic configs, not much could be validated by `ConfigCommand`
>> (see https://issues.apache.org/jira/browse/KAFKA-4788, for example).
>>
>> Ismael
>>
>> On Mon, May 8, 2017 at 5:39 AM, James Cheng  wrote:
>>
>>> The KIP talks about allowing the user to provide a AlterConfigsPolicy. I
>>> assume that even if the user does not provide a custom policy, that there
>>> are some basic validation that the broker will do by default?
>>>
>>> The reason I'm asking is I'm thinking ahead to
>>> https://issues.apache.org/jira/browse/KAFKA-4680, and wanted to know if
>>> there is going to be central place where we can do checks for stuff like
>>> that.
>>>
>>> -James
>>>
>>> > On May 4, 2017, at 10:32 PM, Ismael Juma  wrote:
>>> >
>>> > Hi all,
>>> >
>>> > We've posted "KIP-133: List and Alter Configs Admin APIs" for
>>> discussion:
>>> >
>>> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-133%3A
>>> +List+and+Alter+Configs+Admin+APIs
>>> >
>>> > This completes the first batch of AdminClient APIs so that topic,
>>> config
>>> > and ACL management is supported.
>>> >
>>> > Please take a look. Your feedback is appreciated.
>>> >
>>> > Thanks,
>>> > Ismael
>>>
>>>
>>
>

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Ismael Juma]

Quick update, I renamed ListAcls to DescribeAcls (and related classes and
methods) as that is more consistent with other protocols (like ListGroups
and DescribeGroups).

Ismael

On Mon, May 8, 2017 at 2:17 PM, Ismael Juma  wrote:

> Hi James,
>
> Yes, we will have a place where we perform validation independently of the
> policy. This will be quite basic in the initial version, but we can improve
> it over time. We will be able to do better than `ConfigCommand` because we
> will have access to the broker topic configs. Without access to the broker
> topic configs, not much could be validated by `ConfigCommand` (see
> https://issues.apache.org/jira/browse/KAFKA-4788, for example).
>
> Ismael
>
> On Mon, May 8, 2017 at 5:39 AM, James Cheng  wrote:
>
>> The KIP talks about allowing the user to provide a AlterConfigsPolicy. I
>> assume that even if the user does not provide a custom policy, that there
>> are some basic validation that the broker will do by default?
>>
>> The reason I'm asking is I'm thinking ahead to
>> https://issues.apache.org/jira/browse/KAFKA-4680, and wanted to know if
>> there is going to be central place where we can do checks for stuff like
>> that.
>>
>> -James
>>
>> > On May 4, 2017, at 10:32 PM, Ismael Juma  wrote:
>> >
>> > Hi all,
>> >
>> > We've posted "KIP-133: List and Alter Configs Admin APIs" for
>> discussion:
>> >
>> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-133%
>> 3A+List+and+Alter+Configs+Admin+APIs
>> >
>> > This completes the first batch of AdminClient APIs so that topic, config
>> > and ACL management is supported.
>> >
>> > Please take a look. Your feedback is appreciated.
>> >
>> > Thanks,
>> > Ismael
>>
>>
>

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Ismael Juma]

Hi James,

Yes, we will have a place where we perform validation independently of the
policy. This will be quite basic in the initial version, but we can improve
it over time. We will be able to do better than `ConfigCommand` because we
will have access to the broker topic configs. Without access to the broker
topic configs, not much could be validated by `ConfigCommand` (see
https://issues.apache.org/jira/browse/KAFKA-4788, for example).

Ismael

On Mon, May 8, 2017 at 5:39 AM, James Cheng  wrote:

> The KIP talks about allowing the user to provide a AlterConfigsPolicy. I
> assume that even if the user does not provide a custom policy, that there
> are some basic validation that the broker will do by default?
>
> The reason I'm asking is I'm thinking ahead to https://issues.apache.org/
> jira/browse/KAFKA-4680, and wanted to know if there is going to be
> central place where we can do checks for stuff like that.
>
> -James
>
> > On May 4, 2017, at 10:32 PM, Ismael Juma  wrote:
> >
> > Hi all,
> >
> > We've posted "KIP-133: List and Alter Configs Admin APIs" for discussion:
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 133%3A+List+and+Alter+Configs+Admin+APIs
> >
> > This completes the first batch of AdminClient APIs so that topic, config
> > and ACL management is supported.
> >
> > Please take a look. Your feedback is appreciated.
> >
> > Thanks,
> > Ismael
>
>

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[James Cheng]

The KIP talks about allowing the user to provide a AlterConfigsPolicy. I assume 
that even if the user does not provide a custom policy, that there are some 
basic validation that the broker will do by default?

The reason I'm asking is I'm thinking ahead to 
https://issues.apache.org/jira/browse/KAFKA-4680, and wanted to know if there 
is going to be central place where we can do checks for stuff like that.

-James

> On May 4, 2017, at 10:32 PM, Ismael Juma  wrote:
> 
> Hi all,
> 
> We've posted "KIP-133: List and Alter Configs Admin APIs" for discussion:
> 
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-133%3A+List+and+Alter+Configs+Admin+APIs
> 
> This completes the first batch of AdminClient APIs so that topic, config
> and ACL management is supported.
> 
> Please take a look. Your feedback is appreciated.
> 
> Thanks,
> Ismael

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Ismael Juma]

Thanks for the feedback Colin. Comments inline.

On Sun, May 7, 2017 at 9:29 PM, Colin McCabe  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.

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.

Also, I realised that I was a bit hasty when I changed Config to
Collection 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.).

Here's the full diff of the changes:

https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=68719318=14=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.

Thanks.
Ismael

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Colin McCabe]

On Fri, May 5, 2017, at 19:24, Ismael Juma wrote:
> Hi Colin,
> 
> Thanks for the review. Comments inline.
> 
> On Fri, May 5, 2017 at 9:45 PM, Colin McCabe  wrote:
> 
> > As Jun commented, it's probably more consistent to use
> > Collection, unless this really needs to be an ordered
> > list.  That way people can use any container they want.
> >
> 
> Agreed, updated the KIP.
> 
> > public class ListConfigsResult {
> > >public KafkaFuture> all();
> > > }
> >
> > This should be Map, right?  Since we
> > can have multiple configurations per entity.
> >
> 
> Yes, indeed.
> 
> With the "all" function, if any listing of a ConfigEntity got an error,
> > the whole future completes exceptionally and you get no results for
> > anything.  Which is fine in some cases, but not what you want in others.
> >  So it would be nice to have a function in ListConfigsResult like:
> >
> > >public Map>> configs();
> >
> 
> I considered that, but the proposed protocol cannot fail on a per entity
> basis during listing. So, I thought we could leave this method out for
> now.
> We could introduce it if per entity failures ever became possible. What
> do
> you think?

Hi Ismael,

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.)

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.

> 
> > A new ACL resource type `Configs` will be introduced with Describe, Alter
> > > and All operations. Initially, this resource type will work at a global
> > > level like the Cluster resource type. That is, the only valid resource
> > > name will be "kafka-cluster"
> >
> > Hmm.  Having a new "kafka-cluster" resource seems like it duplicates the
> > existing Cluster resource, which is also a singleton and also represents
> > the whole cluster.
> 
> 
> I was thinking of it as the Configs resource for all of the cluster as
> opposed to the Configs resource for a specific entity. So, in the future,
> the resource name could represent specific entities. Given that, I don't
> think it duplicates the existing Cluster resource.
> 
> 
> > Configurations don't really seem like a separate
> > resource type like topics or brokers-- they seem like an aspect of the
> > existing resources.
> 
> 
> Well, a broker is part of the cluster resource, but also a resource
> itself.
> So, I think it's reasonable to apply the same principle to configs.

While it's true that broker resources are a part of the cluster
resource, the brokers that we have in a cluster can change over time,
and are different for each cluster.  This makes it interesting to talk
about brokers separately from clusters.  Topics are similar-- every
cluster will have a different set of topics, so there is a clear reason
to have topic resources that are separate from the cluster resource.

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.

> 
> What about instead having an action for reading configs and an action
> > for writing configs.  So having READ_CONFIGS on the CLUSTER resource
> > would give you the ability to read all configs, and having WRITE_CONFIGS
> > on the CLUSTER resource would give you the ability to write all configs.
> >  And being superuser, or having ALL would give you both READ_CONFIGS and
> > WRITE_CONFIGS.  Later on we can make it finer grained by granting
> > READ_CONFIGS on specific TOPIC objects for specific principals, and so
> > on.
> >
> 
> Not sure. It seems like operations are meant to be generic (ClusterAction
> is probably the exception) whereas READ_CONFIGS seems to combine an
> operation and a resource (although you did say that you don't see Configs
> as a resource).

I think it's better to have specific operations than generic ones, since
it gives administrators finer-grained control and makes it clearer what
the operations mean.  Generic-sounding operations leave people guessing
what they actually do.

It seems natural to say that having ALL on a resource like a broker
should grant you the ability to READ_CONFIGS / WRITE_CONFIGS on that
broker as well.  Some generic operations should contain more specific
ones, like we do today with 

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Ismael Juma]

Hi James,

On Sun, May 7, 2017 at 8:36 AM, James Cheng  wrote:

> Makes sense, and I don't have any concerns about it. Actually, I'm excited
> for the capability. That means you can completely tell how a topic is
> configured, without needing to have any knowledge of how the broker is
> configured.
>

That's the goal indeed.

Unrelated to this KIP, but important to keep in mind is that all brokers
must have the same topic configs for things to work in a sane manner. That
is a peculiarity of how topic configs work in Kafka: the fallback to broker
topic configs is done during load, not during store. This makes it possible
to update all topic config defaults by updating the config in every broker
following by a rolling restart (or by upgrading to a new version of Kafka
where broker topic config defaults has changed), but it can also be a
source of weird behaviour if the broker topic configs are not consistent.
There are a few ways this could be improved, but that's a subject for
another KIP. :)

You said that "if we fallback to the broker config (whether it's a default
> or not), is_default will be true.". I assume that if I were to ListConfig
> on the *broker*, then if a config is the kafka default, then is_default
> will be true. And if the operator specifically set an override for the
> broker config, then is_default will be false?
>

That's right.

If that is all correct, then you can also completely tell how a broker is
> configured, without needing to look at its config file. Which is pretty
> convenient.
>

Yeah, that is indeed the goal as well. :)

I'll update the KIP to clarify these important points.

Thanks,
Ismael

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[James Cheng]

> On May 6, 2017, at 11:27 AM, Ismael Juma  wrote:
> 
> Hi James,
> 
> Yes, that's right, it will return all config values. For topic configs,
> that means falling back to the respective broker config value, which could
> also be a default. If we fallback to the broker config (whether it's a
> default or not), is_default will be true. Does this make sense? And do you
> have any concerns related to this?
> 

Ismael,

Makes sense, and I don't have any concerns about it. Actually, I'm excited for 
the capability. That means you can completely tell how a topic is configured, 
without needing to have any knowledge of how the broker is configured.

You said that "if we fallback to the broker config (whether it's a default or 
not), is_default will be true.". I assume that if I were to ListConfig on the 
*broker*, then if a config is the kafka default, then is_default will be true. 
And if the operator specifically set an override for the broker config, then 
is_default will be false?

If that is all correct, then you can also completely tell how a broker is 
configured, without needing to look at its config file. Which is pretty 
convenient.

-James


> Ismael
> 
> On Sat, May 6, 2017 at 6:19 AM, James Cheng  wrote:
> 
>> Hi Ismael,
>> 
>> Thanks for the KIP.
>> 
>> I see that in the ListConfigs Response protocol, that configs have an
>> is_default field. Does that mean that it will include *all* config values,
>> instead of just overridden ones?
>> 
>> As an example, kafka-config.sh --describe on a topic will, right now, only
>> show overridden configs. With ListConfigs, will it show all default configs
>> for the topic, which includes the configs that were inherited from the
>> broker configs (which themselves, might also be defaults)?
>> 
>> Thanks,
>> -James
>> 
>>> On May 4, 2017, at 7:32 PM, Ismael Juma  wrote:
>>> 
>>> Hi all,
>>> 
>>> We've posted "KIP-133: List and Alter Configs Admin APIs" for discussion:
>>> 
>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> 133%3A+List+and+Alter+Configs+Admin+APIs
>>> 
>>> This completes the first batch of AdminClient APIs so that topic, config
>>> and ACL management is supported.
>>> 
>>> Please take a look. Your feedback is appreciated.
>>> 
>>> Thanks,
>>> Ismael
>> 
>> 

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Ismael Juma]

Hi James,

Yes, that's right, it will return all config values. For topic configs,
that means falling back to the respective broker config value, which could
also be a default. If we fallback to the broker config (whether it's a
default or not), is_default will be true. Does this make sense? And do you
have any concerns related to this?

Ismael

On Sat, May 6, 2017 at 6:19 AM, James Cheng  wrote:

> Hi Ismael,
>
> Thanks for the KIP.
>
> I see that in the ListConfigs Response protocol, that configs have an
> is_default field. Does that mean that it will include *all* config values,
> instead of just overridden ones?
>
> As an example, kafka-config.sh --describe on a topic will, right now, only
> show overridden configs. With ListConfigs, will it show all default configs
> for the topic, which includes the configs that were inherited from the
> broker configs (which themselves, might also be defaults)?
>
> Thanks,
> -James
>
> > On May 4, 2017, at 7:32 PM, Ismael Juma  wrote:
> >
> > Hi all,
> >
> > We've posted "KIP-133: List and Alter Configs Admin APIs" for discussion:
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 133%3A+List+and+Alter+Configs+Admin+APIs
> >
> > This completes the first batch of AdminClient APIs so that topic, config
> > and ACL management is supported.
> >
> > Please take a look. Your feedback is appreciated.
> >
> > Thanks,
> > Ismael
>
>

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[James Cheng]

Hi Ismael,

Thanks for the KIP.

I see that in the ListConfigs Response protocol, that configs have an 
is_default field. Does that mean that it will include *all* config values, 
instead of just overridden ones?

As an example, kafka-config.sh --describe on a topic will, right now, only show 
overridden configs. With ListConfigs, will it show all default configs for the 
topic, which includes the configs that were inherited from the broker configs 
(which themselves, might also be defaults)?

Thanks,
-James

> On May 4, 2017, at 7:32 PM, Ismael Juma  wrote:
> 
> Hi all,
> 
> We've posted "KIP-133: List and Alter Configs Admin APIs" for discussion:
> 
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-133%3A+List+and+Alter+Configs+Admin+APIs
> 
> This completes the first batch of AdminClient APIs so that topic, config
> and ACL management is supported.
> 
> Please take a look. Your feedback is appreciated.
> 
> Thanks,
> Ismael

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Ismael Juma]

Hi Colin,

Thanks for the review. Comments inline.

On Fri, May 5, 2017 at 9:45 PM, Colin McCabe  wrote:

> As Jun commented, it's probably more consistent to use
> Collection, unless this really needs to be an ordered
> list.  That way people can use any container they want.
>

Agreed, updated the KIP.

> public class ListConfigsResult {
> >public KafkaFuture> all();
> > }
>
> This should be Map, right?  Since we
> can have multiple configurations per entity.
>

Yes, indeed.

With the "all" function, if any listing of a ConfigEntity got an error,
> the whole future completes exceptionally and you get no results for
> anything.  Which is fine in some cases, but not what you want in others.
>  So it would be nice to have a function in ListConfigsResult like:
>
> >public Map>> configs();
>

I considered that, but the proposed protocol cannot fail on a per entity
basis during listing. So, I thought we could leave this method out for now.
We could introduce it if per entity failures ever became possible. What do
you think?

> A new ACL resource type `Configs` will be introduced with Describe, Alter
> > and All operations. Initially, this resource type will work at a global
> > level like the Cluster resource type. That is, the only valid resource
> > name will be "kafka-cluster"
>
> Hmm.  Having a new "kafka-cluster" resource seems like it duplicates the
> existing Cluster resource, which is also a singleton and also represents
> the whole cluster.


I was thinking of it as the Configs resource for all of the cluster as
opposed to the Configs resource for a specific entity. So, in the future,
the resource name could represent specific entities. Given that, I don't
think it duplicates the existing Cluster resource.


> Configurations don't really seem like a separate
> resource type like topics or brokers-- they seem like an aspect of the
> existing resources.


Well, a broker is part of the cluster resource, but also a resource itself.
So, I think it's reasonable to apply the same principle to configs.

What about instead having an action for reading configs and an action
> for writing configs.  So having READ_CONFIGS on the CLUSTER resource
> would give you the ability to read all configs, and having WRITE_CONFIGS
> on the CLUSTER resource would give you the ability to write all configs.
>  And being superuser, or having ALL would give you both READ_CONFIGS and
> WRITE_CONFIGS.  Later on we can make it finer grained by granting
> READ_CONFIGS on specific TOPIC objects for specific principals, and so
> on.
>

Not sure. It seems like operations are meant to be generic (ClusterAction
is probably the exception) whereas READ_CONFIGS seems to combine an
operation and a resource (although you did say that you don't see Configs
as a resource).

> public class Config {
> >public Config(String name, String value) {
> >this(name, value, false, false, false);
> >}
>
> We have a bunch of classes like ProducerConfig, ConsumerConfig,
> AdminClientConfig, that are basically bags of key->value mappings.  But
> this class represents a single key value mapping, which seems
> inconsistent.  Does it make sense to call it "ConfigEntry" or
> "ConfigPair" instead?
>

ConfigEntry sounds good.

> public class ConfigEntity {
> >enum Type {
> >CLIENT, BROKER, TOPIC, UNKNOWN;
> >}
> >public ConfigEntity(Type type, String name) { ... }
>
> This seems really similar to AclResource, which is basically a tuple of
> (AclResourceType, String).  Currently AclResourceType can be UNKNOWN,
> ANY, TOPIC, GROUP, CLUSTER.


If you take into account KIP-98 (i.e.
https://github.com/apache/kafka/pull/2979)), it's UNKNOWN, ANY, TOPIC,
GROUP, CLUSTER, PRODUCER_TRANSACTIONAL_ID, PRODUCER_ID for AclResourceType.

It feels like these are two classes to
> represent the same concept, and maybe we should just unify them.  We
> could just rename AclResource/AclResourceType to Resource/ResourceType
> and use them in both KIP-140 and this KIP.
>

I'm not sure. The intersection of valid values in both for 0.11.0.0 would
be UNKNOWN and TOPIC (out of 9 valid values in the union of both). In the
future, we would probably have configs for CLUSTER as well. It's not clear
to me that the benefit of trying to unify these enums is worth it. I can't
imagine that we'd ever allow configs to be listed or altered on PRODUCER_ID
or PRODUCER_TRANSACTIONAL_ID, for example.

In the end, it's easy enough to add a value to the relevant enum if we
extend functionality and to me it seems clearer if we keep them separate.
I'd be interested in other opinions. In any case, this is a detail and we
can tweak it in either direction during the PR review, I think.

Ismael

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Ismael Juma]

Hi Jun,

Thanks for the review.

1. Yes, a Set would work too. I updated to take Collection for consistency
as Colin said.
2. That was a typo, it should have been a boolean indicating whether we
should only perform validation. Updated the KIP.

Thanks,
Ismael

On Fri, May 5, 2017 at 6:35 PM, Jun Rao  wrote:

> Hi, Ismael,
>
> Thanks for the KIP. Looks good overall. Just a couple of minor comments.
>
> 1. Should AdminClient.listConfigs() take a set of ConfigEntity instead of a
> list?
>
> 2. validateOnly(boolean timeout): Should the time be Integer?
>
> Jun
>
>
> On Thu, May 4, 2017 at 7:32 PM, Ismael Juma  wrote:
>
> > Hi all,
> >
> > We've posted "KIP-133: List and Alter Configs Admin APIs" for discussion:
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 133%3A+List+and+Alter+Configs+Admin+APIs
> >
> > This completes the first batch of AdminClient APIs so that topic, config
> > and ACL management is supported.
> >
> > Please take a look. Your feedback is appreciated.
> >
> > Thanks,
> > Ismael
> >
>

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Colin McCabe]

Hi Ismael,

Thanks for the KIP.  This will be a great improvement.

> public class AdminClient {
>public ListConfigsResult listConfigs(List entities,
> ListConfigsOptions options);
>public AlterConfigsResult alterConfigs(Map 
> configs,
>   AlterConfigsOptions options);
> }

As Jun commented, it's probably more consistent to use
Collection, unless this really needs to be an ordered
list.  That way people can use any container they want.

> public class ListConfigsResult {
>public KafkaFuture> all();
> }

This should be Map, right?  Since we
can have multiple configurations per entity.

With the "all" function, if any listing of a ConfigEntity got an error,
the whole future completes exceptionally and you get no results for
anything.  Which is fine in some cases, but not what you want in others.
 So it would be nice to have a function in ListConfigsResult like:

>public Map>> configs();

> A new ACL resource type `Configs` will be introduced with Describe, Alter
> and All operations. Initially, this resource type will work at a global
> level like the Cluster resource type. That is, the only valid resource
> name will be "kafka-cluster"

Hmm.  Having a new "kafka-cluster" resource seems like it duplicates the
existing Cluster resource, which is also a singleton and also represents
the whole cluster.  Configurations don't really seem like a separate
resource type like topics or brokers-- they seem like an aspect of the
existing resources.

What about instead having an action for reading configs and an action
for writing configs.  So having READ_CONFIGS on the CLUSTER resource
would give you the ability to read all configs, and having WRITE_CONFIGS
on the CLUSTER resource would give you the ability to write all configs.
 And being superuser, or having ALL would give you both READ_CONFIGS and
WRITE_CONFIGS.  Later on we can make it finer grained by granting
READ_CONFIGS on specific TOPIC objects for specific principals, and so
on.

> public class Config {
>public Config(String name, String value) {
>this(name, value, false, false, false);
>}

We have a bunch of classes like ProducerConfig, ConsumerConfig,
AdminClientConfig, that are basically bags of key->value mappings.  But
this class represents a single key value mapping, which seems
inconsistent.  Does it make sense to call it "ConfigEntry" or
"ConfigPair" instead?

> public class ConfigEntity {
>enum Type {
>CLIENT, BROKER, TOPIC, UNKNOWN;
>}
>public ConfigEntity(Type type, String name) { ... }

This seems really similar to AclResource, which is basically a tuple of
(AclResourceType, String).  Currently AclResourceType can be UNKNOWN,
ANY, TOPIC, GROUP, CLUSTER.  It feels like these are two classes to
represent the same concept, and maybe we should just unify them.  We
could just rename AclResource/AclResourceType to Resource/ResourceType
and use them in both KIP-140 and this KIP.

best,
Colin

On Fri, May 5, 2017, at 10:35, Jun Rao wrote:
> Hi, Ismael,
> 
> Thanks for the KIP. Looks good overall. Just a couple of minor comments.
> 
> 1. Should AdminClient.listConfigs() take a set of ConfigEntity instead of
> a
> list?
> 
> 2. validateOnly(boolean timeout): Should the time be Integer?
> 
> Jun
> 
> 
> On Thu, May 4, 2017 at 7:32 PM, Ismael Juma  wrote:
> 
> > Hi all,
> >
> > We've posted "KIP-133: List and Alter Configs Admin APIs" for discussion:
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 133%3A+List+and+Alter+Configs+Admin+APIs
> >
> > This completes the first batch of AdminClient APIs so that topic, config
> > and ACL management is supported.
> >
> > Please take a look. Your feedback is appreciated.
> >
> > Thanks,
> > Ismael
> >

Re: [DISCUSS] KIP-133: List and Alter Configs Admin APIs

[Jun Rao]

Hi, Ismael,

Thanks for the KIP. Looks good overall. Just a couple of minor comments.

1. Should AdminClient.listConfigs() take a set of ConfigEntity instead of a
list?

2. validateOnly(boolean timeout): Should the time be Integer?

Jun


On Thu, May 4, 2017 at 7:32 PM, Ismael Juma  wrote:

> Hi all,
>
> We've posted "KIP-133: List and Alter Configs Admin APIs" for discussion:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 133%3A+List+and+Alter+Configs+Admin+APIs
>
> This completes the first batch of AdminClient APIs so that topic, config
> and ACL management is supported.
>
> Please take a look. Your feedback is appreciated.
>
> Thanks,
> Ismael
>