Hi Viktor, Where are we with this KIP? Is it just waiting for votes? We should try and get this in earlier in the release cycle this time.
Thank you, Rajini On Mon, May 21, 2018 at 7:44 AM, Viktor Somogyi <viktorsomo...@gmail.com> wrote: > 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 > >>> >>> > > >> >> >> > > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > >>> >>> > > >> >> >> > > > > > > > >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > > >>> >>> > > >> >> >> > > > > >>> >>> > > >> >> >> > > >>> >>> > > >>> > >
