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 <cmcc...@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 <cmcc...@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
>
