Hi Viktor, 3. Agree that it would be better to use something like ConfigEntityList rather than ListQuotas. But I would leave it out for now since we are so close to KIP freeze. We can introduce it later if required. Earlier, I was thinking that if we just wanted to get a list of entities without their actual quota values, you could have an option in DescribeQuotas to return the entities without the quota values. But actually that doesn't make sense since you will need to read all the ZK entries and find the ones with quotas in the first place. So let's just leave DescribeQuotas as-is.
7. Yes, with client-id quotas at the lowest level. The full list in the order of precedence is here: https://kafka.apache.org/documentation/#design_quotasconfig 9. One more suggestion. Since DescribeQuotas and AlterQuotas are specific to quotas, we could use *quota* instead of *config* in the protocol (and AdminClient API). Instead of *config_name*, we could use a *quota_type* enum (we have three types). And *config_value *could be *quota_value *that is a double rather than a string*,* On Thu, Jan 18, 2018 at 9:38 AM, Viktor Somogyi <viktorsomo...@gmail.com> wrote: > Hi Rajini, > > 1. Yes, --adminclient.config it is. I missed that, sorry :) > > 3. Indeed SCRAM in this case can raise complications. Since we'd like to > handle altering SCRAM credentials via AlterConfigs, I think we should use > DescribeConfigs to describe them. That is, to describe all the entities of > a given type we might need to introduce some kind of generic way of getting > metadata of config entities instead of ListQuotas which is very specific. > Therefore instead of ListQuotas we could have a ConfigMetadata (or > ConfigEntityList) protocol tailored to the needs of the admin client. This > protocol would accept a list of config entities for the request and produce > a list of entities as a response. For instance requesting (type:USER > name:user1, type:CLIENT name:) resources would return all the clients of > user1. This I think would better fit the use case and potentially future > use case too (like 'group' that you mentioned). > What do you think? Should we introduce a protocol like this or shall we > solve the problem without it? > Also, in a previous email you mentioned that we could use options. Could > you please elaborate on this idea? > > 7. Yea, that is a good idea. How are quotas applied? Does it first fall > back to the default on the same level and if there is no default then > applies the parent's config? > > Regards, > Viktor > > > On Wed, Jan 17, 2018 at 12:56 PM, Rajini Sivaram <rajinisiva...@gmail.com> > wrote: > > > Hi Viktor, > > > > Thank you for the responses. > > > > 1. ConsoleProducer uses *--producer.config <file> --producer-property > > key=value*, ConsoleConsumer uses* --consumer.config <file> > > --consumer-property key=value*, so perhaps we should use > > *--adminclient.config > > *rather than *--config.properties*? > > > > 3. The one difference is that with ListGroups, ListTopics etc. you are > > listing the entities (groups/topics). With ConfigCommand, you can list > > entities and that makes sense. But with ListQuotas, quota is an > attribute, > > the entity is user/clientid/(user, clientId). This is significant since > we > > can store other attributes for those entities. For instance, we store > SCRAM > > credentials along with quotas for 'user'. So to ListQuotas for user, you > > need to actually get the entries and check if quotas are defined or just > > credentials. > > > > 7. > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > 226+-+Dynamic+Broker+Configuration > > is > > replacing* is_default *flag in the config entry for DescribeConfigs with > a* > > config_source* enum which indicates where the config came from. Perhaps > we > > could do something similar here? > > > > 8. Yes, that is correct. > > > > Regards, > > > > Rajini > > > > On Tue, Jan 16, 2018 at 2:59 PM, Viktor Somogyi <viktorsomo...@gmail.com > > > > wrote: > > > > > Hi Rajini, > > > > > > 1 and 2: corrected it in my code. So there will be 3 properties in this > > > group: --bootstrap-server, --config.properties and > --adminclient-property > > > (following the conventions established elsewhere, like the > > > console-producer). > > > > > > 3: Let me explain the reason for ListQuotas. In the current version of > > > kafka-configs you can do this: > > > bin/kafka-configs.sh --zookeeper localhost:2181 --describe > --entity-type > > > users > > > And this will return you all the configs for users under /config/users > > > znode. In that command you have direct access to zookeeper, so you can > > > instantly do an iteration through the znode. Therefore I looked at > other > > > protocols (ListAcls ListGroups, ListTopics) and thought it would be > > aligned > > > with those if I separated off listing. This has the pros of being able > to > > > return a list of entities or fine tuning permissions (you might have > > users > > > who don't have to know other users' quota settings). Once the list of > > > resources returned, the user can initiate a bulk describe. > > > Of course the cons of having ListQuotas as a separate protocol is that > it > > > might do something too simple for a protocol and actually as you say it > > > might be implemented with DescribeQuotasOptions perhaps by only using > an > > > extra flag in the DescribeQuotas protocol (like "describe_all"). > > > Do you think it would be better to add an option to "describe all"? > Also > > of > > > course the response would be "asymmetric" to the request in this case > > > meaning that I send one resource and might get back more. One of my > > reasons > > > of implementing this "list then describe" way of doing things was to be > > > aligned with DescribeConfigs (as that is also symmetric similarly). > > > > > > 4. OK, I think I can do that, it makes sense. > > > > > > 5. Sure, I can do that. In fact I started with this but then reverted > as > > I > > > didn't know if it's really planned to have more levels. > > > > > > 6. OK, will remove those. > > > > > > 7. Well, at this point if you specify (userA, client1) it will simply > > get's > > > the znode's data at /config/users/userA/clients/client1 . If there is > no > > > such client it returns empty. This now functionally compatible with the > > > current ConfigCommand. However what you're saying I think makes sense, > > > meaning that we want to return what will actually be applied if the > exact > > > mapping doesn't exist (and may tell the user that for instance "there > is > > no > > > client1 for userA so I returned that will be applied"). > > > > > > 8. Certainly, I can include that as well. Just to confirm, are you > > thinking > > > about something like this? > > > bin/kafka-configs.sh --zookeeper localhost:2181 --alter --add-config > > > 'SCRAM-SHA-256=[iterations=8192,password=alice-secret],SCRAM > > > -SHA-512=[password=alice-secret]' > > > --entity-type users --entity-name alice > > > Which will result in: > > > get /config/users/alice > > > {"version":1,"config":{"SCRAM-SHA-512":"salt=some_salt,store > > > d_key=some_key,server_key=some_server_key,iterations= > > > 4096","SCRAM-SHA-256":"salt=some_salt,stored_key=some_key_ > > > 2,server_key=some_server_key_2,iterations=8192"}} > > > > > > Regards, > > > Viktor > > > > > > > > > On Tue, Jan 16, 2018 at 12:33 PM, Rajini Sivaram < > > rajinisiva...@gmail.com> > > > wrote: > > > > > > > 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 > > > > > > > > > > > > > > >
