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 <ja...@confluent.io> 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 <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 > > >
