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 <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?
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 DESCRIBE and READ, etc, and ALL.
>
> > 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.
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.
Colin
>
> Ismael
