Hi Viktor, Thanks for the updates. Looks good, just a few minor comments:
1. AdminOperation - could be AlterOperation since it is only applied to 'Alter'? 2. Don't think we need `Unknown` type to process old requests. We can use `Set` as the default for alter requests with version 0. 3. There is a typo in https://cwiki.apache.org/confluence/display/KAFKA/KIP-248+-+Create+New+ConfigCommand+That+Uses+The+New+AdminClient#KIP-248-CreateNewConfigCommandThatUsesTheNewAdminClient-AdminClientAPIs : AdminOperation enum has a constructor QuotaType. On Wed, Feb 7, 2018 at 4:53 PM, Viktor Somogyi <viktorsomo...@gmail.com> wrote: > Hi Rajini, > > I think it makes sense absolutely and even we could do it for AlterQuotas > as we will have the same problem there. > Updated my KIP > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-248+-+Create+New+ > ConfigCommand+That+Uses+The+New+AdminClient> > to > reflect these changes: > - proposed protocol changes > - created a AdminOperation type to represent the Add/Set/Delete triplet. > (Put in the org.apache.kafka.clients.admin package) > > Please let me know if I missed something that you thought otherwise. > > Regards, > Viktor > > > On Tue, Feb 6, 2018 at 1:31 PM, Rajini Sivaram <rajinisiva...@gmail.com> > wrote: > > > Hi Viktor, > > > > While implementing KAFKA-6494, I realised that there is a mismatch > between > > the --alter command of ConfigCommand and AlterConfigs request. > > ConfigCommand uses --add-config and --delete-config to make incremental > > updates. --add-config reads all the configs from ZooKeeper and adds the > > delta provided on top of that. AlterConfigs request currently sets the > > whole properties object, so you need to know the full set of properties > of > > an entity to use AlterConfigs request through the AdminClient. We don't > > allow sensitive configs to be read using AdminClient, so we can't read > and > > add configs as we do with ZooKeeper. So we need a protocol change to make > > this work. I didn't want to make this change after KIP freeze, so perhaps > > we could include this in your KIP? We could perhaps add a mode > > (SET/ADD/DELETE) for AlterConfigs request where SET matches the existing > > behaviour for backward compatibility and ConfigCommand uses ADD/DELETE. > > > > Thoughts? > > > > Regards, > > > > Rajini > > > > On Fri, Jan 19, 2018 at 12:57 PM, Viktor Somogyi < > viktorsomo...@gmail.com> > > wrote: > > > > > Hi Rajini, > > > > > > Ok, I think I got you. I wasn't calculating with the fact that the > parent > > > might not be set, therefore it could be a default user as well or even > > the > > > default client if nothing else is set (supposing we're talking about > the > > > <user, client> example). So if I'm correct, the quota will be applied > in > > > the order of the above points. In this case your argument is absolutely > > > valid. I'll modify the QuotaSource. > > > > > > About your last point: yes, I was hesitating a lot. I thought the > > interface > > > would be simpler but after removing the helpers it's not that scary > > > afterall :). > > > I'll start the vote. > > > > > > Viktor > > > > > > > > > On Thu, Jan 18, 2018 at 7:59 PM, Rajini Sivaram < > rajinisiva...@gmail.com > > > > > > wrote: > > > > > > > Hi Viktor, > > > > > > > > Thanks for the updates. > > > > > > > > *QuotaSource* currently has *Self/Default/Parent*. Not sure if that > is > > > > sufficient. > > > > For the entity <user, client-id>, quota could be used from any of > these > > > > configs: > > > > > > > > 1. /config/users/<user>/clients/<client-id> > > > > 2. /config/users/<user>/clients/<default> > > > > 3. /config/users/<user> > > > > 4. /config/users/<default>/clients/<client-id> > > > > 5. /config/users/<default>/clients/<default> > > > > 6. /config/users/<default> > > > > 7. /config/clients/<client-id> > > > > 8. /config/clients/<default> > > > > > > > > So perhaps we should have a *QuotaSource* entry for each of these > > eight? > > > > > > > > A couple of minor points: > > > > > > > > - *Help Message* still uses --config.properties > > > > - The other AdminClient APIs don't use aliases for various > > > collections. > > > > So not sure if we need the aliases here. I think you can leave it > > > as-is > > > > and > > > > see what others think. > > > > > > > > Yes, please do start the voting thread to make it in time for the KIP > > > > freeze. > > > > > > > > Thank you, > > > > > > > > Rajini > > > > > > > > > > > > On Thu, Jan 18, 2018 at 6:15 PM, Viktor Somogyi < > > viktorsomo...@gmail.com > > > > > > > > wrote: > > > > > > > > > Rajini, I have updated the KIP as agreed. Could you please have a > > > second > > > > > look at it? > > > > > I have also added a section about SCRAM: > > > > > "To enable describing and altering SCRAM credentials we will use > the > > > > > DescribeConfigs and AlterConfigs protocols. There are no changes in > > the > > > > > protocol's structure but we will allow the USER resource type to be > > > > passed > > > > > in the protocol. When this happens, the server will know that SCRAM > > > > configs > > > > > are asked and will send them in the response. In case of > > AlterConfigs > > > > if a > > > > > USER resource type is passed it will validate if there are only > SCRAM > > > > > credentials are changed. If not, then will fail with > > > > > InvalidRequestException > > > > > ." > > > > > > > > > > If you don't have any comments, we might start voting as we're > close > > to > > > > KIP > > > > > freeze. > > > > > > > > > > On Thu, Jan 18, 2018 at 12:12 PM, Viktor Somogyi < > > > > viktorsomo...@gmail.com> > > > > > wrote: > > > > > > > > > > > 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 > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > >
