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 >> >>> > > >> >> >> > > > > > > > > > > > >> >>> > > >> >> >> > > > > > > > > > >> >>> > > >> >> >> > > > > > > > > >> >>> > > >> >> >> > > > > > > > >> >>> > > >> >> >> > > > > > > >> >>> > > >> >> >> > > > > > >> >>> > > >> >> >> > > > > >> >>> > > >> >> >> > > > >> >>> > > >> >> >> > >> >>> > >>
