Hi Ismael,
Thanks for the KIP. This will be a great improvement.
> public class AdminClient {
> public ListConfigsResult listConfigs(List<ConfigEntity> entities,
> ListConfigsOptions options);
> public AlterConfigsResult alterConfigs(Map<ConfigEntity, List<Config>>
> configs,
> AlterConfigsOptions options);
> }
As Jun commented, it's probably more consistent to use
Collection<ConfigEntity>, unless this really needs to be an ordered
list. That way people can use any container they want.
> public class ListConfigsResult {
> public KafkaFuture<Map<ConfigEntity, Config>> all();
> }
This should be Map<ConfigEntity, Collection<Config>>, 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<ConfigEntity, KafkaFuture<Collection<Config>>>> 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 <ism...@juma.me.uk> 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
> >
