Hi All, I'd like to ask the community to please vote for this as the KIP freeze is tomorrow.
Thank you very much, Viktor On Mon, May 21, 2018 at 9:39 AM, Viktor Somogyi <viktorsomo...@gmail.com> wrote: > Hi Colin, > > Sure, I'll add a note. > Thanks for your vote. > > Viktor > > On Sat, May 19, 2018 at 1:01 AM, Colin McCabe <cmcc...@apache.org> wrote: >> Hi Viktor, >> >> Thanks, this looks good. >> >> The boolean should default to false if not set, to ensure that existing >> clients continue to work as-is, right? Might be good to add a note >> specifying that. >> >> +1 (non-binding) >> >> best, >> Colin >> >> On Fri, May 18, 2018, at 08:16, Viktor Somogyi wrote: >>> Updated KIP-248: >>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-248+-+Create+New+ConfigCommand+That+Uses+The+New+AdminClient >>> >>> I'd like to ask project members, committers and contributors to vote >>> as this would be a useful improvement in Kafka. >>> >>> Sections changed: >>> - Public interfaces: added the bin/scram-credentials.sh command that >>> we discussed with Colin. >>> - Wire format types: removed AlterOperations. As discussed with Colin, >>> we don't actually need this: we should behave in an incremental way in >>> AlterQuotas. For AlterConfig we'll implement this behavior with an >>> extra flag on the protocol (and incrementing the version). >>> - AlterQuotas protocol: removed AlterOperations. Added some more >>> description to the behavior of the protocol. Removing quotas will >>> happen by sending a NaN instead of the AlterOperations. (Since IEEE >>> 754 covers NaNs and it is not a valid config for any quota, I think it >>> is a good notation.) >>> - SCRAM: so it will be done by the scram-credentials command that uses >>> direct zookeeper connection. I think further modes, like doing it >>> through the broker is not necessary. The idea here is that zookeeper >>> in this case acts as a credentials store. This should be decoupled >>> from the broker as we manage broker credentials as well. The new >>> command acts as a client to the store. >>> - AlterConfigs will have an incremental_update flag as discussed. By >>> default it is false to provide the backward compatible behavior. When >>> it is true it will merge the configs with what's there in the node. >>> Deletion in incremental mode is done by sending an empty string as >>> config value. >>> - Other compatibility changes: this KIP doesn't scope listing multiple >>> users and client's quotas. As per a conversation with Rajini, it is >>> not a common use case and we can add it back later if it is needed. If >>> this functionality is needed, the old code should be still available >>> through run-kafka-class. (Removed the USE_OLD_KAFKA_CONFIG_COMMAND as >>> it doesn't make sense anymore.) >>> >>> On Fri, May 18, 2018 at 12:33 PM, Viktor Somogyi >>> <viktorsomo...@gmail.com> wrote: >>> > Ok, ignore my previous mail (except the last sentence), gmail didn't >>> > update me about your last email :/. >>> > >>> >> I think we should probably just create a flag for alterConfigs which >>> >> marks it as incremental, like we discussed earlier, and do this as a >>> >> compatible change that is needed for the shell command. >>> > >>> > Alright, I missed that about the sensitive configs too, so in this >>> > case I can agree with this. I'll update the KIP this afternoon and >>> > update this thread. >>> > Thanks again for your contribution. >>> > >>> > Viktor >>> > >>> > On Fri, May 18, 2018 at 2:34 AM, Colin McCabe <cmcc...@apache.org> wrote: >>> >> Actually, I just realized that this won't work. The AlterConfigs API is >>> >> kind of broken right now. DescribeConfigs won't return the "sensitive" >>> >> configurations like passwords. So doing describe + edit + alter will >>> >> wipe out all sensitive configs. :( >>> >> >>> >> I think we should probably just create a flag for alterConfigs which >>> >> marks it as incremental, like we discussed earlier, and do this as a >>> >> compatible change that is needed for the shell command. >>> >> >>> >> best, >>> >> Colin >>> >> >>> >> >>> >> On Thu, May 17, 2018, at 09:32, Colin McCabe wrote: >>> >>> Hi Viktor, >>> >>> >>> >>> Since the KIP freeze is coming up really soon, maybe we should just drop >>> >>> the section about changes to AlterConfigs from KIP-248. We don't really >>> >>> need it here, since ConfigCommand can use AlterConfigs as-is. >>> >>> >>> >>> We can pick up the discussion about improving AlterConfigs in a future >>> >>> KIP. >>> >>> >>> >>> cheers, >>> >>> Colin >>> >>> >>> >>> On Wed, May 16, 2018, at 22:06, Colin McCabe wrote: >>> >>> > Hi Viktor, >>> >>> > >>> >>> > The shell command isn’t that easy to integrate into applications. >>> >>> > AdminClient will get integrated into a lot more stuff, which >>> >>> > increases the potential for conflicts. I would argue that we should >>> >>> > fix this soon. >>> >>> > If we do want to reduce the scope in this KIP, we could do the merge >>> >>> > in >>> >>> > the ConfigCommand tool for now, and leave AC unchanged. >>> >>> > Best, >>> >>> > Colin >>> >>> > >>> >>> > >>> >>> > On Wed, May 16, 2018, at 04:57, Viktor Somogyi wrote: >>> >>> > > Hi Colin, >>> >>> > > >>> >>> > > > Doing get-merge-set is buggy, though. If someone else does >>> >>> > > > get-merge- >>> >>> > > > set at the same time as you, you might overwrite that person's >>> >>> > > > changes, or vice versa. So I really don't think we should try to >>> >>> > > > do >>> >>> > > > this. Also, having both an incremental and a full API is useful, >>> >>> > > > and it's just a single boolean at the protocol and API level.> >>> >>> > > Overwriting somebody's change is currently possible with the >>> >>> > > ConfigCommand, as it will do this get-merge-set behavior on the >>> >>> > > client> side, in the command. From this perspective I think it's >>> >>> > > not much >>> >>> > > different to do this with the admin client. Also I think admins >>> >>> > > don't> modify the quotas/configs of a client/user/topic/broker >>> >>> > > often (and >>> >>> > > multiple admins would do it even more rarely), so I don't think it >>> >>> > > is> a big issue. What I think would be useful here but may be out >>> >>> > > of scope> is to version the changes similarly to leader epochs. So >>> >>> > > when an admin> updates the configs, it will increment a version >>> >>> > > number and won't let> other admins to push changes in with lower >>> >>> > > than that. Instead it would> return an error. >>> >>> > > >>> >>> > > I would be also interested what others think about this? >>> >>> > > >>> >>> > > Cheers, >>> >>> > > Viktor >>> >>> > > >>> >>> > > >>> >>> > > On Sat, May 12, 2018 at 2:29 AM, Colin McCabe >>> >>> > > <cmcc...@apache.org> wrote:> > On Wed, May 9, 2018, at 05:41, >>> >>> > > Viktor Somogyi wrote: >>> >>> > > >> Hi Colin, >>> >>> > > >> >>> >>> > > >> > We are going to need to create a new version of >>> >>> > > >> > AlterConfigsRequest to add the "incremental" boolean. So while >>> >>> > > >> > we're doing that, maybe we can change the type to >>> >>> > > >> > NULLABLE_STRING.> >> >>> >>> > > >> I was just talking to a colleague yesterday and we came to the >>> >>> > > >> conclusion that we should keep the boolean flag only on the >>> >>> > > >> client> >> side (as you may have suggested earlier?) and not >>> >>> > > >> make part of the> >> protocol as it might lead to a very >>> >>> > > >> complicated API on the long >>> >>> > > >> term.> >> Also we would keep the server side API simpler. >>> >>> > > >> Instead of the >>> >>> > > >> protocol change we could just simply have the boolean flag in >>> >>> > > >> AlterConfigOptions and the AdminClient should do the >>> >>> > > >> get-merge-set> >> logic which corresponds to the behavior of the >>> >>> > > >> current >>> >>> > > >> ConfigCommand.> >> That way we won't need to change the protocol >>> >>> > > >> for now but >>> >>> > > >> still have> >> both functionality. What do you think? >>> >>> > > > >>> >>> > > > Hi Viktor, >>> >>> > > > >>> >>> > > > Doing get-merge-set is buggy, though. If someone else does >>> >>> > > > get-merge- >>> >>> > > > set at the same time as you, you might overwrite that person's >>> >>> > > > changes, or vice versa. So I really don't think we should try to >>> >>> > > > do >>> >>> > > > this. Also, having both an incremental and a full API is useful, >>> >>> > > > and it's just a single boolean at the protocol and API level.> > >>> >>> > > >> >>> >>> > > >> > Hmm. Not sure I follow. KIP-133 doesn't use the empty string >>> >>> > > >> > or >>> >>> > > >> > "<default>" to indicate defaults, does it?> >> >>> >>> > > >> No it doesn't. It was just my early idea to indicate "delete" >>> >>> > > >> on the> >> protocol level. (We are using <default> for denoting >>> >>> > > >> the default >>> >>> > > >> client id or user in zookeeper.) Rajini was referring that we >>> >>> > > >> shouldn't expose this to the protocol level but instead denote >>> >>> > > >> delete> >> with an empty string. >>> >>> > > >> >>> >>> > > >> > This comes from DescribeConfigsResponse. >>> >>> > > >> > Unless I'm missing something, I think your suggestion to not >>> >>> > > >> > expose "<default>" is already implemented?> >> >>> >>> > > >> In some way, yes. Although this one is used in describe and not >>> >>> > > >> in> >> alter. For alter I don't think we'd need my early >>> >>> > > >> "<default>" idea.> > >>> >>> > > > OK. Thanks for the explanation. Using an empty string to >>> >>> > > > indicate >>> >>> > > > delete, as Rajini suggested, seems pretty reasonable to me. null >>> >>> > > > would work as well.> > >>> >>> > > >> >>> >>> > > >> >> And we use STRING rather than NULLABLE_STRING in describe >>> >>> > > >> >> configs etc. So we> >> >> should probably do the same for >>> >>> > > >> >> quotas." >>> >>> > > >> > >>> >>> > > >> > I think nearly all responses treat ERROR_MESSAGE as a nullable >>> >>> > > >> > string. CommonFields#ERROR_MESSAGE, which is used by most of >>> >>> > > >> > them, is a nullable string. It's DescribeConfigsResponse that >>> >>> > > >> > is >>> >>> > > >> > the black sheep here.> >> > >>> >>> > > >> > > public static final Field.NullableStr ERROR_MESSAGE = >>> >>> > > >> > new >>> >>> > > >> > > Field.NullableStr("error_message", "Response error >>> >>> > > >> > > message");> >> >>> >>> > > >> Looking at DescribeConfigsResponse (and AlterConfigsResponse) >>> >>> > > >> they use> >> nullable_string in the code. KIP-133 states >>> >>> > > >> otherwise though. So in> >> this case it's not a problem luckily. >>> >>> > > > >>> >>> > > > Thanks for finding this inconsistency. I'll change the KIP to >>> >>> > > > reflect what was actually implemented (nullable string for >>> >>> > > > error).> > >>> >>> > > > cheers, >>> >>> > > > Colin >>> >>> > > > >>> >>> > > >> >>> >>> > > >> > What about writing a small script that just handles setting up >>> >>> > > >> > SCRAM credentials? It would probably be easier to maintain >>> >>> > > >> > than >>> >>> > > >> > the old config command. Otherwise we have to explain when each >>> >>> > > >> > tool should be used, which will be confusing to users.> >> >>> >>> > > >> I'd like that, yes :). >>> >>> > > >> >>> >>> > > >> Cheers, >>> >>> > > >> Viktor >>> >>> > > >> >>> >>> > > >> On Mon, May 7, 2018 at 6:52 PM, Colin McCabe <cmcc...@apache.org> >>> >>> > > >> wrote:> >> > On Fri, May 4, 2018, at 05:49, Viktor Somogyi wrote: >>> >>> > > >> >> Hi Colin, >>> >>> > > >> >> >>> >>> > > >> >> > Rather than breaking compatibility, we should simply add a >>> >>> > > >> >> > new >>> >>> > > >> >> > "incremental" boolean to AlterConfigsOptions. Callers can >>> >>> > > >> >> > set >>> >>> > > >> >> > this boolean to true when they want the update to be >>> >>> > > >> >> > incremental. It should default to false so that old code >>> >>> > > >> >> > continues to work.> >> >> >>> >>> > > >> >> Agreed, let's do it this way. >>> >>> > > >> >> >>> >>> > > >> >> > Hmm. I don't think AlterOperation is necessary. If the >>> >>> > > >> >> > user >>> >>> > > >> >> > wants to delete a configuration key named "foo", they can >>> >>> > > >> >> > create a ConfigEntry with name = "foo", value = null.> >> >> >>> >>> > > >> >> AlterConfig's config type currently is string, so the only >>> >>> > > >> >> possibility> >> >> is to use an empty string as changing the >>> >>> > > >> >> type to >>> >>> > > >> >> nullable_string> >> >> could be breaking if the client code >>> >>> > > >> >> doesn't expect -1 as the >>> >>> > > >> >> string> >> >> size. In the discussion thread earlier we had a >>> >>> > > >> >> conversation >>> >>> > > >> >> about> >> >> this with Rajini, let me paste it here (so it >>> >>> > > >> >> gives some >>> >>> > > >> >> context). At> >> >> that point I had the text "<default>" for >>> >>> > > >> >> this functionality: >>> >>> > > >> > >>> >>> > > >> > Hi Viktor, >>> >>> > > >> > >>> >>> > > >> > We are going to need to create a new version of >>> >>> > > >> > AlterConfigsRequest to add the "incremental" boolean. So while >>> >>> > > >> > we're doing that, maybe we can change the type to >>> >>> > > >> > NULLABLE_STRING.> >> > >>> >>> > > >> >> "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.> >> > >>> >>> > > >> > Hmm. Not sure I follow. KIP-133 doesn't use the empty string >>> >>> > > >> > or >>> >>> > > >> > "<default>" to indicate defaults, does it?> >> > >>> >>> > > >> > There is a ConfigEntry class: >>> >>> > > >> > >>> >>> > > >> > > @InterfaceStability.Evolving >>> >>> > > >> > > public class ConfigEntry { >>> >>> > > >> > > >>> >>> > > >> > > private final String name; >>> >>> > > >> > > private final String value; >>> >>> > > >> > > private final ConfigSource source; >>> >>> > > >> > > private final boolean isSensitive; >>> >>> > > >> > > private final boolean isReadOnly; >>> >>> > > >> > > private final List<ConfigSynonym> synonyms; >>> >>> > > >> > >>> >>> > > >> > and the ConfigSource enum indicates where the config came from: >>> >>> > > >> > >>> >>> > > >> > > /** >>> >>> > > >> > > * Source of configuration entries. >>> >>> > > >> > > */ >>> >>> > > >> > > public enum ConfigSource { >>> >>> > > >> > > DYNAMIC_TOPIC_CONFIG, // dynamic topic >>> >>> > > >> > > config that is configured for a specific topic> >> >>> >>> > > >> > > > DYNAMIC_BROKER_CONFIG, // dynamic broker >>> >>> > > >> > > config that is configured for a specific broker> >> >>> >>> > > >> > > > DYNAMIC_DEFAULT_BROKER_CONFIG, // dynamic broker >>> >>> > > >> > > config that is configured as default for all brokers >>> >>> > > >> > > in the cluster> >> > > >>> >>> > > >> > STATIC_BROKER_CONFIG, // static broker >>> >>> > > >> > > config provided as broker properties at start up >>> >>> > > >> > (e.g. >>> >>> > > >> > > server.properties file)> >> > > >>> >>> > > >> > DEFAULT_CONFIG, // built-in default >>> >>> > > >> > > configuration for configs that have a default >>> >>> > > >> > value> >> > > UNKNOWN // >>> >>> > > >> > source unknown e.g. >>> >>> > > >> > > in the ConfigEntry used for alter requests where >>> >>> > > >> > > source is not set> >> > > } >>> >>> > > >> > >>> >>> > > >> > This comes from DescribeConfigsResponse. >>> >>> > > >> > Unless I'm missing something, I think your suggestion to not >>> >>> > > >> > expose "<default>" is already implemented?> >> > >>> >>> > > >> >> And we use STRING rather than NULLABLE_STRING in describe >>> >>> > > >> >> configs etc. So we> >> >> should probably do the same for >>> >>> > > >> >> quotas." >>> >>> > > >> > >>> >>> > > >> > I think nearly all responses treat ERROR_MESSAGE as a nullable >>> >>> > > >> > string. CommonFields#ERROR_MESSAGE, which is used by most of >>> >>> > > >> > them, is a nullable string. It's DescribeConfigsResponse that >>> >>> > > >> > is >>> >>> > > >> > the black sheep here.> >> > >>> >>> > > >> > > public static final Field.NullableStr ERROR_MESSAGE = >>> >>> > > >> > new >>> >>> > > >> > > Field.NullableStr("error_message", "Response error >>> >>> > > >> > > message");> >> > >>> >>> > > >> >> >>> >>> > > >> >> > Yeah, this might be an excessive maintenance burden. Maybe >>> >>> > > >> >> > we >>> >>> > > >> >> > should get rid of the old zookeeper-based code, and just >>> >>> > > >> >> > move >>> >>> > > >> >> > towards having only a KIP-248-based tool. It's a breaking >>> >>> > > >> >> > change, but it's clear to users that it's occurring, and >>> >>> > > >> >> > what >>> >>> > > >> >> > the fix is (specifying --bootstrap-server instead of -- >>> >>> > > >> >> > zookeeper).> >> >> >>> >>> > > >> >> Earlier Rajini raised a concern that direct zookeeper >>> >>> > > >> >> interaction is> >> >> required to add the SCRAM credentials >>> >>> > > >> >> which will be used for >>> >>> > > >> >> validation if inter-broker communication uses this auth >>> >>> > > >> >> method. >>> >>> > > >> >> This> >> >> is currently done by the ConfigCommand. Therefore >>> >>> > > >> >> we can't >>> >>> > > >> >> completely> >> >> get rid of it yet either. >>> >>> > > >> >> >>> >>> > > >> >> In my opinion though on a longer term (and this is now a bit >>> >>> > > >> >> off-topic) Kafka shouldn't use Zookeeper as a credentials >>> >>> > > >> >> store, >>> >>> > > >> >> just> >> >> provide an interface, so 3rd party authentication >>> >>> > > >> >> stores could >>> >>> > > >> >> be> >> >> implemented. Then similarly to the authorizer we >>> >>> > > >> >> could have >>> >>> > > >> >> Zookeeper> >> >> as a default though and a client that >>> >>> > > >> >> manages SCRAM credentials >>> >>> > > >> >> in ZK.> >> >> From this perspective I'd leave the the command >>> >>> > > >> >> there but put a> >> >> warning that the tool is deprecated >>> >>> > > >> >> and should only be used for> >> >> setting up SCRAM >>> >>> > > >> >> credentials. >>> >>> > > >> >> What do you think? >>> >>> > > >> > >>> >>> > > >> > What about writing a small script that just handles setting up >>> >>> > > >> > SCRAM credentials? It would probably be easier to maintain >>> >>> > > >> > than >>> >>> > > >> > the old config command. Otherwise we have to explain when each >>> >>> > > >> > tool should be used, which will be confusing to users.> >> > >>> >>> > > >> > best, >>> >>> > > >> > Colin >>> >>> > > >> > >>> >>> > > >> >> >>> >>> > > >> >> Cheers, >>> >>> > > >> >> Viktor >>> >>> > > >> >> >>> >>> > > >> >> >>> >>> > > >> >> On Thu, May 3, 2018 at 7:47 PM, Colin McCabe >>> >>> > > >> >> <cmcc...@apache.org> wrote:> >> >> > On Thu, May 3, 2018, at >>> >>> > > >> >> 05:11, Viktor Somogyi wrote: >>> >>> > > >> >> >> @Magnus, yes that is correct. Thanks for your feedback. >>> >>> > > >> >> >> Updated it with> >> >> >> this (which might be subject to >>> >>> > > >> >> >> change based on the >>> >>> > > >> >> >> conversation with> >> >> >> Colin): "The changes done will >>> >>> > > >> >> >> be incremental in version 1, >>> >>> > > >> >> >> opposed to the> >> >> >> atomic behavior in version 0. For >>> >>> > > >> >> >> instance in version 0 >>> >>> > > >> >> >> sending an update> >> >> >> for producer_byte_rate for >>> >>> > > >> >> >> userA would result in removing all >>> >>> > > >> >> >> previous data> >> >> >> and setting userA's config with >>> >>> > > >> >> >> producer_byte_rate. Now in >>> >>> > > >> >> >> version 1> >> >> >> opposed to version 0 it will add an >>> >>> > > >> >> >> extra config and keeps >>> >>> > > >> >> >> other existing> >> >> >> configs." >>> >>> > > >> >> > >>> >>> > > >> >> > Hi Viktor, >>> >>> > > >> >> > >>> >>> > > >> >> > AdminClient#alterConfigs is a public API which users have >>> >>> > > >> >> > already written code against. If we silently change what it >>> >>> > > >> >> > does to be incremental addition rather than complete >>> >>> > > >> >> > replacement of the existing configuration, we will break all >>> >>> > > >> >> > of that existing code. If we do that, there is not even any >>> >>> > > >> >> > way that users can write code to support both broker >>> >>> > > >> >> > versions. >>> >>> > > >> >> > AdminClient does not expose any API that users can use to >>> >>> > > >> >> > check broker version. I think that would be really bad for >>> >>> > > >> >> > users.> >> >> > >>> >>> > > >> >> > Rather than breaking compatibility, we should simply add a >>> >>> > > >> >> > new >>> >>> > > >> >> > "incremental" boolean to AlterConfigsOptions. Callers can >>> >>> > > >> >> > set >>> >>> > > >> >> > this boolean to true when they want the update to be >>> >>> > > >> >> > incremental. It should default to false so that old code >>> >>> > > >> >> > continues to work.> >> >> > >>> >>> > > >> >> >> >>> >>> > > >> >> >> @Colin, >>> >>> > > >> >> >> yes, I have/had a hard time finding a place for this >>> >>> > > >> >> >> operation. I think ADD> >> >> >> and DELETE should be on >>> >>> > > >> >> >> config level to allow complex use >>> >>> > > >> >> >> cases (if someone> >> >> >> builds their own tool based on >>> >>> > > >> >> >> the AdminClient), so users can >>> >>> > > >> >> >> add and> >> >> >> delete multiple configs in one request. >>> >>> > > >> >> > >>> >>> > > >> >> > Hmm. I don't think AlterOperation is necessary. If the >>> >>> > > >> >> > user >>> >>> > > >> >> > wants to delete a configuration key named "foo", they can >>> >>> > > >> >> > create a ConfigEntry with name = "foo", value = null.> >> >>> >>> > > >> >> > >> > >>> >>> > > >> >> >> But also at the same time, SET is as you're suggesting >>> >>> > > >> >> >> really >>> >>> > > >> >> >> seems like a> >> >> >> flag that tells the >>> >>> > > >> >> >> AdminClient/AdminManager how they should >>> >>> > > >> >> >> behave.> >> >> >> However since the AdminClient matches >>> >>> > > >> >> >> protocol version with >>> >>> > > >> >> >> the broker via> >> >> >> the API_VERSIONS request, I think >>> >>> > > >> >> >> it would be enough to >>> >>> > > >> >> >> modify the> >> >> >> AdminManager that it should behave >>> >>> > > >> >> >> differently in case of an >>> >>> > > >> >> >> increased> >> >> >> protocol versions, if there is this >>> >>> > > >> >> >> extra flag set through >>> >>> > > >> >> >> AlterConfigOptions (AdminClient sets the flag on the >>> >>> > > >> >> >> protocol, which will> >> >> >> be reflected after parsing >>> >>> > > >> >> >> in AdminManager). Also if we >>> >>> > > >> >> >> target this change> >> >> >> to 2.0 (June?), then we might >>> >>> > > >> >> >> not need the extra flag but >>> >>> > > >> >> >> make the behavior> >> >> >> break. What do you think? >>> >>> > > >> >> > >>> >>> > > >> >> > Right. I think a flag in AlterConfigsRequest makes sense. >>> >>> > > >> >> > AdminClient can set it based on a boolean field in >>> >>> > > >> >> > AlterConfigsOptions.> >> >> > >>> >>> > > >> >> >> >>> >>> > > >> >> >> Keeping the --zookeeper option working is not infeasible of >>> >>> > > >> >> >> course - and as> >> >> >> per the community's feedback it >>> >>> > > >> >> >> may be the better option. >>> >>> > > >> >> >> Although one of> >> >> >> the goals is to put this new >>> >>> > > >> >> >> ConfigCommand to the tools >>> >>> > > >> >> >> module, which> >> >> >> doesn't have the dependency on the >>> >>> > > >> >> >> server code, it would be a >>> >>> > > >> >> >> bit harder.> >> >> >> Most likely I'd need to call into >>> >>> > > >> >> >> the Scala code with >>> >>> > > >> >> >> reflection, which> >> >> >> could be quite complicated. >>> >>> > > >> >> > >>> >>> > > >> >> > Yeah, this might be an excessive maintenance burden. Maybe >>> >>> > > >> >> > we >>> >>> > > >> >> > should get rid of the old zookeeper-based code, and just >>> >>> > > >> >> > move >>> >>> > > >> >> > towards having only a KIP-248-based tool. It's a breaking >>> >>> > > >> >> > change, but it's clear to users that it's occurring, and >>> >>> > > >> >> > what >>> >>> > > >> >> > the fix is (specifying --bootstrap-server instead of -- >>> >>> > > >> >> > zookeeper).> >> >> > >>> >>> > > >> >> > best, >>> >>> > > >> >> > Colin >>> >>> > > >> >> > >>> >>> > > >> >> >> >>> >>> > > >> >> >> Also rewrote the request semantics, hopefully it's more >>> >>> > > >> >> >> clear >>> >>> > > >> >> >> now.> >> >> >> >>> >>> > > >> >> >> Let me know what do you think about this and thank you for >>> >>> > > >> >> >> your feedback.> >> >> >> >>> >>> > > >> >> >> Cheers, >>> >>> > > >> >> >> Viktor >>> >>> > > >> >> >> >>> >>> > > >> >> >> On Thu, Apr 26, 2018 at 7:06 PM, Colin McCabe >>> >>> > > >> >> >> <cmcc...@apache.org> wrote:> >> >> >> >>> >>> > > >> >> >> > Hi Viktor, >>> >>> > > >> >> >> > >>> >>> > > >> >> >> > If I'm reading the KIP right, it looks like the new >>> >>> > > >> >> >> > proposed verison of> >> >> >> > AlterConfigs sets an >>> >>> > > >> >> >> > OperationType on a per-config basis: >>> >>> > > >> >> >> > >>> >>> > > >> >> >> > > AlterConfigs Request (Version: 1) => [resources] >>> >>> > > >> >> >> > > validate_only> >> >> >> > > validate_only => >>> >>> > > >> >> >> > BOOLEAN >>> >>> > > >> >> >> > > resources => resource_type resource_name [configs] >>> >>> > > >> >> >> > > resource_type => INT8 >>> >>> > > >> >> >> > > resource_name => STRING >>> >>> > > >> >> >> > > configs => config_name config_value >>> >>> > > >> >> >> > config_operation> >> >> >> > > config_name => >>> >>> > > >> >> >> > STRING >>> >>> > > >> >> >> > > config_value => STRING >>> >>> > > >> >> >> > > config_operation => INT8 [NEW ADDITION] >>> >>> > > >> >> >> > > >>> >>> > > >> >> >> > > Request Semantics: >>> >>> > > >> >> >> > > >>> >>> > > >> >> >> > > By default in the broker we parse an >>> >>> > > >> >> >> > > AlterConfigRequest version 0> >> >> >> > > with >>> >>> > > >> >> >> > Unknown operation and handle it with the currently >>> >>> > > >> >> >> > > existing> >> >> >> > behavior. >>> >>> > > >> >> >> > > Version 1 requests however must have the operation set >>> >>> > > >> >> >> > > to other than> >> >> >> > > Unknown, otherwise an >>> >>> > > >> >> >> > InvalidRequestException will be >>> >>> > > >> >> >> > > thrown.> >> >> >> > > Set operation also >>> >>> > > >> >> >> > does Add if needed to be >>> >>> > > >> >> >> > > backward> >> >> >> > compatible >>> >>> > > >> >> >> > > with the existing ConfigCommand semantics. >>> >>> > > >> >> >> > >>> >>> > > >> >> >> > However, this seems like a configuration that should be >>> >>> > > >> >> >> > global to the> >> >> >> > whole AlterConfigs request, >>> >>> > > >> >> >> > right? It doesn't make sense >>> >>> > > >> >> >> > to have one> >> >> >> > configuration key use >>> >>> > > >> >> >> > AlterOperation.Set and the other use> >> >> >> > >>> >>> > > >> >> >> > AlterOperation.Add -- the Set one specifies that we >>> >>> > > >> >> >> > should >>> >>> > > >> >> >> > overwrite the> >> >> >> > whole node in ZK. >>> >>> > > >> >> >> > >>> >>> > > >> >> >> > Another consideration here is that we should do this in a >>> >>> > > >> >> >> > compatible> >> >> >> > fashion in AdminClient. Existing >>> >>> > > >> >> >> > code that relies on the >>> >>> > > >> >> >> > "set everything"> >> >> >> > behavior should not break. >>> >>> > > >> >> >> > The best way to do this is to >>> >>> > > >> >> >> > add a boolean to> >> >> >> > >>> >>> > > >> >> >> > ./clients/src/main/java/org/apache/kafka/clients/admin/Alt- >>> >>> > > >> >> >> > erConfigsOptions.java> >> >> >> > , specifying whether >>> >>> > > >> >> >> > we want to clear everything that >>> >>> > > >> >> >> > hasn't been> >> >> >> > specified, or not. This should >>> >>> > > >> >> >> > default to true so that >>> >>> > > >> >> >> > existing code can> >> >> >> > continue to work.... >>> >>> > > >> >> >> > Unless we believe that the existing >>> >>> > > >> >> >> > AlterConfigs> >> >> >> > behavior is so broken that it >>> >>> > > >> >> >> > should be changed, even in a> >> >> >> > >>> >>> > > >> >> >> > compatibility-breaking way. >>> >>> > > >> >> >> > >>> >>> > > >> >> >> > Similarly, for other tools, we managed to support both >>> >>> > > >> >> >> > the >>> >>> > > >> >> >> > zookeeper-based> >> >> >> > way and the new way in the >>> >>> > > >> >> >> > same tool for a while. This >>> >>> > > >> >> >> > seems like> >> >> >> > something users would really >>> >>> > > >> >> >> > want-- is it truly infeasible >>> >>> > > >> >> >> > to do here? The> >> >> >> > Java code could call into >>> >>> > > >> >> >> > the Scala code as necessary when >>> >>> > > >> >> >> > the zk flag was> >> >> >> > specified, right? >>> >>> > > >> >> >> > >>> >>> > > >> >> >> > best, >>> >>> > > >> >> >> > Colin >>> >>> > > >> >> >> > >>> >>> > > >> >> >> > >>> >>> > > >> >> >> > On Thu, Apr 26, 2018, at 01:47, Magnus Edenhill wrote: >>> >>> > > >> >> >> > > Hi Viktor, >>> >>> > > >> >> >> > > >>> >>> > > >> >> >> > > after speaking to Rajini it seems like this KIP will >>> >>> > > >> >> >> > > allow clients to> >> >> >> > > perform incremental >>> >>> > > >> >> >> > > configuration updates with >>> >>> > > >> >> >> > > AlterConfigs, only> >> >> >> > providing >>> >>> > > >> >> >> > > the settings >>> >>> > > >> >> >> > > that it wants to change, as opposed to the current >>> >>> > > >> >> >> > > atomic >>> >>> > > >> >> >> > > behaviour where> >> >> >> > > all settings >>> >>> > > >> >> >> > > need to be provided to avoid having them revert to >>> >>> > > >> >> >> > > their >>> >>> > > >> >> >> > > default values.> >> >> >> > > >>> >>> > > >> >> >> > > If this is indeed the case, could you update the KIP to >>> >>> > > >> >> >> > > make this more> >> >> >> > > clear? >>> >>> > > >> >> >> > > I.e., that using Version 1 of AlterConfigs has the >>> >>> > > >> >> >> > > incremental behaviour,> >> >> >> > > while >>> >>> > > >> >> >> > > version 0 is atomic. >>> >>> > > >> >> >> > > >>> >>> > > >> >> >> > > Thanks, >>> >>> > > >> >> >> > > Magnus >>> >>> > > >> >> >> > > >>> >>> > > >> >> >> > > >>> >>> > > >> >> >> > > >>> >>> > > >> >> >> > > >>> >>> > > >> >> >> > > 2018-04-16 13:27 GMT+02:00 Viktor Somogyi >>> >>> > > >> >> >> > > <viktorsomo...@gmail.com>:> >> >> >> > > >>> >>> > > >> >> >> > > > Hi Rajini, >>> >>> > > >> >> >> > > > >>> >>> > > >> >> >> > > > The current ConfigCommand would still be possible to >>> >>> > > >> >> >> > > > use, therefore> >> >> >> > those >>> >>> > > >> >> >> > > > who wish to set up SCRAM or initial quotas would be >>> >>> > > >> >> >> > > > able to continue> >> >> >> > doing >>> >>> > > >> >> >> > > > it through kafka-run-class.sh. >>> >>> > > >> >> >> > > > In an ideal world I'd keep it in the current >>> >>> > > >> >> >> > > > ConfigCommand command so> >> >> >> > we >>> >>> > > >> >> >> > > > wouldn't mix the zookeeper and admin client configs. >>> >>> > > >> >> >> > > > Perhaps I could> >> >> >> > create >>> >>> > > >> >> >> > > > a kafka-configs-zookeeper.sh shell script for >>> >>> > > >> >> >> > > > shortening the> >> >> >> > > > kafka-run-class >>> >>> > > >> >> >> > > > command. >>> >>> > > >> >> >> > > > What do you and others think? >>> >>> > > >> >> >> > > > >>> >>> > > >> >> >> > > > Thanks, >>> >>> > > >> >> >> > > > Viktor >>> >>> > > >> >> >> > > > >>> >>> > > >> >> >> > > > >>> >>> > > >> >> >> > > > >>> >>> > > >> >> >> > > > On Mon, Apr 16, 2018 at 10:15 AM, Rajini Sivaram < >>> >>> > > >> >> >> > rajinisiva...@gmail.com> >>> >>> > > >> >> >> > > > wrote: >>> >>> > > >> >> >> > > > >>> >>> > > >> >> >> > > > > Hi Viktor, >>> >>> > > >> >> >> > > > > >>> >>> > > >> >> >> > > > > The KIP proposes to remove the ability to configure >>> >>> > > >> >> >> > > > > using ZooKeeper.> >> >> >> > This >>> >>> > > >> >> >> > > > > means we will no longer have the ability to start >>> >>> > > >> >> >> > > > > up >>> >>> > > >> >> >> > > > > a cluster with> >> >> >> > SCRAM >>> >>> > > >> >> >> > > > > credentials since we first need to create SCRAM >>> >>> > > >> >> >> > > > > credentials before> >> >> >> > > > brokers >>> >>> > > >> >> >> > > > > can start if the broker uses SCRAM for inter-broker >>> >>> > > >> >> >> > > > > communication> >> >> >> > and we >>> >>> > > >> >> >> > > > > need SCRAM credentials for the AdminClient before >>> >>> > > >> >> >> > > > > we >>> >>> > > >> >> >> > > > > can create new> >> >> >> > ones. >>> >>> > > >> >> >> > > > > For quotas as well, we will no longer be able to >>> >>> > > >> >> >> > > > > configure quotas> >> >> >> > until >>> >>> > > >> >> >> > > > at >>> >>> > > >> >> >> > > > > least one broker has been started. Perhaps, we >>> >>> > > >> >> >> > > > > ought >>> >>> > > >> >> >> > > > > to retain the> >> >> >> > > > ability >>> >>> > > >> >> >> > > > > to configure using ZooKeeper for these >>> >>> > > >> >> >> > > > > initialization >>> >>> > > >> >> >> > > > > scenarios and> >> >> >> > > > support >>> >>> > > >> >> >> > > > > only AdminClient for dynamic updates? >>> >>> > > >> >> >> > > > > >>> >>> > > >> >> >> > > > > What do others think? >>> >>> > > >> >> >> > > > > >>> >>> > > >> >> >> > > > > Regards, >>> >>> > > >> >> >> > > > > >>> >>> > > >> >> >> > > > > Rajini >>> >>> > > >> >> >> > > > > >>> >>> > > >> >> >> > > > > On Sun, Apr 15, 2018 at 10:28 AM, Ted Yu >>> >>> > > >> >> >> > > > > <yuzhih...@gmail.com>> >> >> >> > wrote: >>> >>> > > >> >> >> > > > > >>> >>> > > >> >> >> > > > > > +1 >>> >>> > > >> >> >> > > > > > -------- Original message --------From: zhenya >>> >>> > > >> >> >> > > > > > Sun >>> >>> > > >> >> >> > > > > > <> >> >> >> > toke...@126.com> >>> >>> > > >> >> >> > > > > > Date: 4/15/18 12:42 AM (GMT-08:00) To: dev >>> >>> > > >> >> >> > > > > > <dev@kafka.apache.org> >> >> >> > > >>> >>> > > >> >> >> > > > Cc: >>> >>> > > >> >> >> > > > > > dev <dev@kafka.apache.org> Subject: Re: [VOTE] #2 >>> >>> > > >> >> >> > > > > > KIP-248: Create> >> >> >> > New >>> >>> > > >> >> >> > > > > > ConfigCommand That Uses The New AdminClient >>> >>> > > >> >> >> > > > > > non-binding +1 >>> >>> > > >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > from my iphone！ >>> >>> > > >> >> >> > > > > > On 04/15/2018 15:41, Attila Sasvári wrote: >>> >>> > > >> >> >> > > > > > Thanks for updating the KIP. >>> >>> > > >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > +1 (non-binding) >>> >>> > > >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > Viktor Somogyi <viktorsomo...@gmail.com> ezt írta >>> >>> > > >> >> >> > > > > > (időpont: 2018.> >> >> >> > ápr. >>> >>> > > >> >> >> > > > > 9., >>> >>> > > >> >> >> > > > > > H 16:49): >>> >>> > > >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > > Hi Magnus, >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > > Thanks for the heads up, added the endianness >>> >>> > > >> >> >> > > > > > > to >>> >>> > > >> >> >> > > > > > > the KIP. Here> >> >> >> > is the >>> >>> > > >> >> >> > > > > > > current text: >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > > "Double >>> >>> > > >> >> >> > > > > > > A new type needs to be added to transfer quota >>> >>> > > >> >> >> > > > > > > values. Since the> >> >> >> > > > > protocol >>> >>> > > >> >> >> > > > > > > classes in Kafka already uses ByteBuffers it is >>> >>> > > >> >> >> > > > > > > logical to use> >> >> >> > their >>> >>> > > >> >> >> > > > > > > functionality for serializing doubles. The >>> >>> > > >> >> >> > > > > > > serialization is> >> >> >> > > > basically a >>> >>> > > >> >> >> > > > > > > representation of the specified floating-point >>> >>> > > >> >> >> > > > > > > value according> >> >> >> > to the >>> >>> > > >> >> >> > > > > > IEEE >>> >>> > > >> >> >> > > > > > > 754 floating-point "double format" bit layout. >>> >>> > > >> >> >> > > > > > > The ByteBuffer> >> >> >> > > > > serializer >>> >>> > > >> >> >> > > > > > > writes eight bytes containing the given double >>> >>> > > >> >> >> > > > > > > value, in Big> >> >> >> > Endian >>> >>> > > >> >> >> > > > > byte >>> >>> > > >> >> >> > > > > > > order, into this buffer at the current >>> >>> > > >> >> >> > > > > > > position, >>> >>> > > >> >> >> > > > > > > and then> >> >> >> > increments >>> >>> > > >> >> >> > > > > the >>> >>> > > >> >> >> > > > > > > position by eight. >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > > The implementation will be defined in >>> >>> > > >> >> >> > > > > > > org.apache.kafka.common.protocol.types with the >>> >>> > > >> >> >> > > > > > > other protocol> >> >> >> > types >>> >>> > > >> >> >> > > > > > and it >>> >>> > > >> >> >> > > > > > > will have no default value much like the other >>> >>> > > >> >> >> > > > > > > types available> >> >> >> > in the >>> >>> > > >> >> >> > > > > > > protocol." >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > > Also, I haven't changed the protocol docs yet >>> >>> > > >> >> >> > > > > > > but >>> >>> > > >> >> >> > > > > > > will do so in> >> >> >> > my PR >>> >>> > > >> >> >> > > > > for >>> >>> > > >> >> >> > > > > > > this feature. >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > > Let me know if you'd still add something. >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > > Regards, >>> >>> > > >> >> >> > > > > > > Viktor >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > > On Mon, Apr 9, 2018 at 3:32 PM, Magnus >>> >>> > > >> >> >> > > > > > > Edenhill <> >> >> >> > mag...@edenhill.se> >>> >>> > > >> >> >> > > > > > > wrote: >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > > > Hi Viktor, >>> >>> > > >> >> >> > > > > > > > >>> >>> > > >> >> >> > > > > > > > since serialization of floats isn't as >>> >>> > > >> >> >> > > > > > > > straight >>> >>> > > >> >> >> > > > > > > > forward as> >> >> >> > > > integers, >>> >>> > > >> >> >> > > > > > > please >>> >>> > > >> >> >> > > > > > > > specify the exact serialization format of >>> >>> > > >> >> >> > > > > > > > DOUBLE in the> >> >> >> > protocol >>> >>> > > >> >> >> > > > docs >>> >>> > > >> >> >> > > > > > > > (e.g., IEEE 754), >>> >>> > > >> >> >> > > > > > > > including endianness (big-endian please). >>> >>> > > >> >> >> > > > > > > > >>> >>> > > >> >> >> > > > > > > > This will help the non-java client ecosystem. >>> >>> > > >> >> >> > > > > > > > >>> >>> > > >> >> >> > > > > > > > Thanks, >>> >>> > > >> >> >> > > > > > > > Magnus >>> >>> > > >> >> >> > > > > > > > >>> >>> > > >> >> >> > > > > > > > >>> >>> > > >> >> >> > > > > > > > 2018-04-09 15:16 GMT+02:00 Viktor Somogyi < >>> >>> > > >> >> >> > viktorsomo...@gmail.com >>> >>> > > >> >> >> > > > >: >>> >>> > > >> >> >> > > > > > > > >>> >>> > > >> >> >> > > > > > > > > Hi Attila, >>> >>> > > >> >> >> > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > 1. It uses ByteBuffers, which in turn will >>> >>> > > >> >> >> > > > > > > > > use> >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > > > > Double.doubleToLongBits >>> >>> > > >> >> >> > > > > > > to >>> >>> > > >> >> >> > > > > > > > > convert the double value to a long and that >>> >>> > > >> >> >> > > > > > > > > long will be> >> >> >> > written >>> >>> > > >> >> >> > > > in >>> >>> > > >> >> >> > > > > > the >>> >>> > > >> >> >> > > > > > > > > buffer. I'v updated the KIP with this. >>> >>> > > >> >> >> > > > > > > > > 2. Good idea, modified it. >>> >>> > > >> >> >> > > > > > > > > 3. During the discussion I remember we >>> >>> > > >> >> >> > > > > > > > > didn't >>> >>> > > >> >> >> > > > > > > > > really decide> >> >> >> > which >>> >>> > > >> >> >> > > > > one >>> >>> > > >> >> >> > > > > > > > would >>> >>> > > >> >> >> > > > > > > > > be the better one but I agree that a >>> >>> > > >> >> >> > > > > > > > > wrapper >>> >>> > > >> >> >> > > > > > > > > class that makes> >> >> >> > > > sure >>> >>> > > >> >> >> > > > > > the >>> >>> > > >> >> >> > > > > > > > list >>> >>> > > >> >> >> > > > > > > > > that is used as a key is immutable is a >>> >>> > > >> >> >> > > > > > > > > good >>> >>> > > >> >> >> > > > > > > > > idea and would> >> >> >> > ease >>> >>> > > >> >> >> > > > > the >>> >>> > > >> >> >> > > > > > > life >>> >>> > > >> >> >> > > > > > > > > of people using the interface. Also more >>> >>> > > >> >> >> > > > > > > > > importantly would> >> >> >> > make >>> >>> > > >> >> >> > > > > sure >>> >>> > > >> >> >> > > > > > > that >>> >>> > > >> >> >> > > > > > > > > we always use the same hashCode. I have >>> >>> > > >> >> >> > > > > > > > > created wrapper> >> >> >> > classes >>> >>> > > >> >> >> > > > for >>> >>> > > >> >> >> > > > > > the >>> >>> > > >> >> >> > > > > > > > map >>> >>> > > >> >> >> > > > > > > > > value as well but that was reverted to keep >>> >>> > > >> >> >> > > > > > > > > things> >> >> >> > consistent. >>> >>> > > >> >> >> > > > > > Although >>> >>> > > >> >> >> > > > > > > > for >>> >>> > > >> >> >> > > > > > > > > the key I think we wouldn't break >>> >>> > > >> >> >> > > > > > > > > consistency. I updated the> >> >> >> > KIP. >>> >>> > > >> >> >> > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > Thanks, >>> >>> > > >> >> >> > > > > > > > > Viktor >>> >>> > > >> >> >> > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > On Tue, Apr 3, 2018 at 1:27 PM, Attila >>> >>> > > >> >> >> > > > > > > > > Sasvári <> >> >> >> > > > > >>> >>> > > >> >> >> > > > > > > > > asasv...@apache.org> >>> >>> > > >> >> >> > > > > > > > > wrote: >>> >>> > > >> >> >> > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > Thanks for working on it Viktor. >>> >>> > > >> >> >> > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > It looks good to me, but I have some >>> >>> > > >> >> >> > > > > > > > > > questions:> >> >> >> > > > > > > > > > - >>> >>> > > >> >> >> > > > > > > > > > I see a new type DOUBLE is used for >>> >>> > > >> >> >> > > > > > > > > > quota_value , and it> >> >> >> > is >>> >>> > > >> >> >> > > > not >>> >>> > > >> >> >> > > > > > > > listed >>> >>> > > >> >> >> > > > > > > > > > among the primitive types on the Kafka >>> >>> > > >> >> >> > > > > > > > > > protocol guide. Can> >> >> >> > you >>> >>> > > >> >> >> > > > > add >>> >>> > > >> >> >> > > > > > > some >>> >>> > > >> >> >> > > > > > > > > > more details? >>> >>> > > >> >> >> > > > > > > > > > - I am not sure that using an environment >>> >>> > > >> >> >> > > > > > > > > > (i.e.> >> >> >> > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > USE_OLD_COMMAND)variable >>> >>> > > >> >> >> > > > > > > > > is >>> >>> > > >> >> >> > > > > > > > > > the best way to control behaviour of >>> >>> > > >> >> >> > > > > > > > > > kafka- >>> >>> > > >> >> >> > > > > > > > > > config.sh . In> >> >> >> > other >>> >>> > > >> >> >> > > > > > > scripts >>> >>> > > >> >> >> > > > > > > > > > (e.g. console-consumer) an argument is >>> >>> > > >> >> >> > > > > > > > > > passed (e.g.> >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > > > > > --new-consumer). >>> >>> > > >> >> >> > > > > > > If >>> >>> > > >> >> >> > > > > > > > > we >>> >>> > > >> >> >> > > > > > > > > > still want to use it, then I would >>> >>> > > >> >> >> > > > > > > > > > suggest >>> >>> > > >> >> >> > > > > > > > > > something like> >> >> >> > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > USE_OLD_KAFKA_CONFIG_COMMAND. What do >>> >>> > > >> >> >> > > > > > > > > > you >>> >>> > > >> >> >> > > > > > > > > > think?> >> >> >> > > > > > > > > > - I >>> >>> > > >> >> >> > > > > > > > > > have seen maps like >>> >>> > > >> >> >> > > > > > > > > > Map<List<ConfigResource>,> >> >> >> > >>> >>> > > >> >> >> > > > > > > > > > > > > > > > Collection<QuotaType>>. >>> >>> > > >> >> >> > > > > > > > > > If List<ConfigResource> is the key type, >>> >>> > > >> >> >> > > > > > > > > > you should make> >> >> >> > sure >>> >>> > > >> >> >> > > > > that >>> >>> > > >> >> >> > > > > > > this >>> >>> > > >> >> >> > > > > > > > > > List is immutable. Have you considered to >>> >>> > > >> >> >> > > > > > > > > > introduce a new> >> >> >> > > > wrapper >>> >>> > > >> >> >> > > > > > > > class? >>> >>> > > >> >> >> > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > Regards, >>> >>> > > >> >> >> > > > > > > > > > - Attila >>> >>> > > >> >> >> > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > On Thu, Mar 29, 2018 at 1:46 PM, zhenya >>> >>> > > >> >> >> > > > > > > > > > Sun >>> >>> > > >> >> >> > > > > > > > > > <> >> >> >> > toke...@126.com> >>> >>> > > >> >> >> > > > > > wrote: >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > +1 （non-binding） >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > | | >>> >>> > > >> >> >> > > > > > > > > > > zhenya Sun >>> >>> > > >> >> >> > > > > > > > > > > 邮箱：toke...@126.com >>> >>> > > >> >> >> > > > > > > > > > > | >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > 签名由 网易邮箱大师 定制 >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > On 03/29/2018 19:40, Sandor Murakozi >>> >>> > > >> >> >> > > > > > > > > > > wrote:> >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > +1 (non-binding) >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > Thanks for the KIP, Viktor >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > On Wed, Mar 21, 2018 at 5:41 PM, Viktor >>> >>> > > >> >> >> > > > > > > > > > > Somogyi <> >> >> >> > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > viktorsomo...@gmail.com >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > wrote: >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > Hi Everyone, >>> >>> > > >> >> >> > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > I've started a vote on KIP-248 >>> >>> > > >> >> >> > > > > > > > > > > > <https://cwiki.apache.org/conf >>> >>> > > >> >> >> > luence/display/KAFKA/KIP- >>> >>> > > >> >> >> > > > > > > > > > 248+-+Create+New+ >>> >>> > > >> >> >> > > > > > > > > > > > ConfigCommand+That+Uses+The+New+AdminC- >>> >>> > > >> >> >> > > > > > > > > > > > lient#KIP-248-> >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > CreateNewConfigCommandThatUsesTheNewAd- >>> >>> > > >> >> >> > > > > > > > > > > > minClient-> >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > DescribeQuotas> >>> >>> > > >> >> >> > > > > > > > > > > > a few weeks ago but at the time I >>> >>> > > >> >> >> > > > > > > > > > > > got a >>> >>> > > >> >> >> > > > > > > > > > > > couple more> >> >> >> > > > comments >>> >>> > > >> >> >> > > > > > and >>> >>> > > >> >> >> > > > > > > it >>> >>> > > >> >> >> > > > > > > > > was >>> >>> > > >> >> >> > > > > > > > > > > > very close to 1.1 feature freeze, >>> >>> > > >> >> >> > > > > > > > > > > > people were occupied> >> >> >> > with >>> >>> > > >> >> >> > > > > > that, >>> >>> > > >> >> >> > > > > > > > so >>> >>> > > >> >> >> > > > > > > > > I >>> >>> > > >> >> >> > > > > > > > > > > > wanted to restart the vote on this. >>> >>> > > >> >> >> > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > *Summary of the KIP* >>> >>> > > >> >> >> > > > > > > > > > > > For those who don't have context I >>> >>> > > >> >> >> > > > > > > > > > > > thought I'd> >> >> >> > summarize it >>> >>> > > >> >> >> > > > > in >>> >>> > > >> >> >> > > > > > a >>> >>> > > >> >> >> > > > > > > > few >>> >>> > > >> >> >> > > > > > > > > > > > sentence. >>> >>> > > >> >> >> > > > > > > > > > > > *Problem & Motivation: *The basic >>> >>> > > >> >> >> > > > > > > > > > > > problem that the KIP> >> >> >> > > > >>> >>> > > >> >> >> > > > > > > > > > > > tries >>> >>> > > >> >> >> > > > > to >>> >>> > > >> >> >> > > > > > > > solve >>> >>> > > >> >> >> > > > > > > > > > is >>> >>> > > >> >> >> > > > > > > > > > > > that kafka-configs.sh (which in turn >>> >>> > > >> >> >> > > > > > > > > > > > uses the> >> >> >> > ConfigCommand >>> >>> > > >> >> >> > > > > > > class) >>> >>> > > >> >> >> > > > > > > > > uses >>> >>> > > >> >> >> > > > > > > > > > a >>> >>> > > >> >> >> > > > > > > > > > > > direct zookeeper connection. This is >>> >>> > > >> >> >> > > > > > > > > > > > not desirable as> >> >> >> > > > >>> >>> > > >> >> >> > > > > > > > > > > > getting >>> >>> > > >> >> >> > > > > > > > around >>> >>> > > >> >> >> > > > > > > > > > the >>> >>> > > >> >> >> > > > > > > > > > > > broker opens up security issues and >>> >>> > > >> >> >> > > > > > > > > > > > prevents the tool> >> >> >> > from >>> >>> > > >> >> >> > > > > > being >>> >>> > > >> >> >> > > > > > > > used >>> >>> > > >> >> >> > > > > > > > > > in >>> >>> > > >> >> >> > > > > > > > > > > > deployments where only the brokers >>> >>> > > >> >> >> > > > > > > > > > > > are >>> >>> > > >> >> >> > > > > > > > > > > > exposed to> >> >> >> > clients. >>> >>> > > >> >> >> > > > > > Also a >>> >>> > > >> >> >> > > > > > > > > > somewhat >>> >>> > > >> >> >> > > > > > > > > > > > smaller motivation is to rewrite the >>> >>> > > >> >> >> > > > > > > > > > > > tool in java as> >> >> >> > part >>> >>> > > >> >> >> > > > of >>> >>> > > >> >> >> > > > > > the >>> >>> > > >> >> >> > > > > > > > > tools >>> >>> > > >> >> >> > > > > > > > > > > > component so we can get rid of >>> >>> > > >> >> >> > > > > > > > > > > > requiring the core> >> >> >> > >>> >>> > > >> >> >> > > > > > > > > > > > module on >>> >>> > > >> >> >> > > > > the >>> >>> > > >> >> >> > > > > > > > > > classpath >>> >>> > > >> >> >> > > > > > > > > > > > for the kafka-configs tool. >>> >>> > > >> >> >> > > > > > > > > > > > *Solution:* >>> >>> > > >> >> >> > > > > > > > > > > > - I've designed new 2 protocols: >>> >>> > > >> >> >> > > > > > > > > > > > DescribeQuotas and> >> >> >> > > > >>> >>> > > >> >> >> > > > > > > > > > > > > > AlterQuotas. >>> >>> > > >> >> >> > > > > > > > > > > > - Also redesigned the output format >>> >>> > > >> >> >> > > > > > > > > > > > of >>> >>> > > >> >> >> > > > > > > > > > > > the command line> >> >> >> > > > >>> >>> > > >> >> >> > > > > > > > > > > > tool >>> >>> > > >> >> >> > > > > so >>> >>> > > >> >> >> > > > > > > it >>> >>> > > >> >> >> > > > > > > > > > provides >>> >>> > > >> >> >> > > > > > > > > > > > a nicer result. >>> >>> > > >> >> >> > > > > > > > > > > > - kafka-configs.[sh/bat] will use a >>> >>> > > >> >> >> > > > > > > > > > > > new >>> >>> > > >> >> >> > > > > > > > > > > > java based> >> >> >> > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > ConfigCommand >>> >>> > > >> >> >> > > > > > > > that >>> >>> > > >> >> >> > > > > > > > > > is >>> >>> > > >> >> >> > > > > > > > > > > > placed in tools. >>> >>> > > >> >> >> > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > I'd be happy to receive any votes or >>> >>> > > >> >> >> > > > > > > > > > > > feedback on this.> >> >> >> > > > > >>> >>> > > >> >> >> > > > > > > > > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > Regards, >>> >>> > > >> >> >> > > > > > > > > > > > Viktor >>> >>> > > >> >> >> > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > >>> >>> > > >> >> >> > > > > > > > >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > >>> >>> > > >> >> >> > > > > >>> >>> > > >> >> >> > > > >>> >>> > > >> >> >> > >>> >>> > >>>
