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. 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. 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.). 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. Thanks. Ismael
