3. Ok, I'll remove this from the KIP for now and perhaps add a future considerations section with the idea.
9. Ok, I can do that. On Thu, Jan 18, 2018 at 11:18 AM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > 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 > > > > > > > > > > > > > > > > > > > > >
