Hi Viktor, Thank you for the KIP. It is looking good. A few comments:
1. --bootstrap-server option: "*Help Message*" uses --bootstrap-servers. I think other tools use the singular form even though it should probably have been plural to start with. Can we use* --bootstrap-server* for consistency? 2. ConsoleProducer/ConsoleConsumer etc. have two ways of providing client properties: *--producer.config <configFile>* and *--producer.property key=value*. Can we do the same here since it is easier for scripts to pass in property on the command line sometimes rather than a file. Perhaps *--adminclient.config *and *--adminclient.property* or something along those lines? 3. Not sure if ListQuotas is useful. For various other entities, we just have a Describe request. Wouldn't that be sufficient? If we did want a less detailed version, we could have options for the describe request. 4. We use "<default>" internally to store default quotas and other defaults. But I don't think we should externalise that string. We use empty string elsewhere for indicating default, we can do the same here. And we use STRING rather than NULLABLE_STRING in describe configs etc. So we should probably do the same for quotas. 5. At the moment, we have two levels of quotas and you can define <user, clientId> quotas. We may want to add more levels in the future, eg. <group, user, clientId>. It would be more flexible to specify an array of entities rather than a single child entity. (e.g resource => [quota_entity], quota_entity => entity_type entity_name). 6. In DescribeQuotasResponse, we don't need the flags read_only or is_sensitve since they will always be false for quotas. 7. What will DescribeQuotas(userA, client1) actually return? Will it return the quota defined for (userA, client1)? Or will it return the quota that will be applied to (userA, client1) - i.e. if there is no quota defined for (userA, client1), but there is a quota defined for userA, will it return userA quota since that will be the quota applied to (userA, client1)? 8. I think at the moment, AdminClient doesn't have an API for creating SCRAM credentials (ConfigCommand is used for this). I think the existing describe/alterConfigs API can be used if we add a resource type USER. If this will be done in the same JIRA, can we include it in the KIP? Regards, Rajini On Fri, Jan 12, 2018 at 11:11 AM, Viktor Somogyi <viktorsomo...@gmail.com> wrote: > Hi all, > > I have submitted KIP-248 that details how can kafka-configs.sh could > utilize KafkaAdminClient through a new ConfigCommand class in the tools > module: > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-248+-+ > Create+New+ConfigCommand+That+Uses+The+New+AdminClient > > This KIP proposes to add 3 new wire protocols (listing quota configs, > describing quota configs and altering quota configs), a new class called > ConfigCommand in tools which would be the main entry point for > kafka-configs.sh and a way to deprecate the current Scala ConfigCommand. > > I'd be happy to receive some feedback about the proposal. > > Thank you & regards, > Viktor >
