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
