Hi Colin,
Thanks for the review. Comments inline.
On Fri, May 5, 2017 at 9:45 PM, Colin McCabe <cmcc...@apache.org> wrote:
> 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.
>
Agreed, updated the KIP.
> 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.
>
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<ConfigEntity, KafkaFuture<Collection<Config>>>> 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
