Hi Magnus, Thanks for pointing that out. I haven't been keeping up-to-date with all the changes and I have no idea how we got here. I had requested the change to use SET/ADD/DELETE way back in February ( http://mail-archives.apache.org/mod_mbox/kafka-dev/201802.mbox/%3cCAOJcB39amNDgf+8EO5fyA0JvpcUEQZBHu=ujhasj4zhjf-b...@mail.gmail.com%3e) and I thought the KIP was updated. Non-incremental updates are pretty much useless for broker config updates, and the KIP as-is doesn't work for broker configs until ADD/DELETE is reintroduced. DescribeConfigs doesn't return sensitive configs, so apart from atomicity, any sequence of changes that requires non-incremental updates simply doesn't work for broker configs.
Hi Viktor, Are you still working on this KIP? Regards, Rajini On Wed, Jul 4, 2018 at 12:14 PM, Magnus Edenhill <mag...@edenhill.se> wrote: > There are some concerns about the incremental option that needs to be > discussed further. > > I believe everyone agrees on the need for incremental updates, allowing a > client > to only alter the configuration it provides in an atomic fashion. > The proposal adds a request-level incremental bool for this purpose, which > is good. > > But I suspect this might not be enough, and thus suggest that we should > extend > the per-config-entry struct with a mode field that tells the broker how to > alter > the given configuration entry: > - append - append value to entry (if no previous value it acts like set) > - set - overwrite value > - delete - delete configuration entry / revert to default. > > If we don't do this, the incremental mode can only be used in "append" > mode, > and a client that wishes to overwrite property A, delete B, and append to > C, > will need to issue three requests: > - 1. DescribeConfigs to get the current config. > - 2. AlterConfigs(incremental=False) to overwrite config property A and > delete B. > - 3. AlterConfigs(incremental=True) to append to config property C. > > This makes the configuration update non-atomic, which incremental is set > out to fix, > any configuration changes made by another client between 1 and 2 would be > lost at 2. > > > This also needs to be exposed in the Admin API to make the user intention > clear, > ConfigEntry should be extended with a new constructor that takes the mode > parameter: append, set, or delete. > The existing constructor should default to set/overwrite (as in the > existing pre-incremental case). > If an application issues an AlterConfigs() request with append or delete > ConfigEntrys and the broker does not support KIP-248, > the request should fail locally in the client. > > For reference, this is how it is exposed in the corresponding C API: > https://github.com/edenhill/librdkafka/blob/master/src/rdkafka.h#L5200 > > > > 2018-07-04 11:28 GMT+02:00 Rajini Sivaram <rajinisiva...@gmail.com>: > > > 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 > > > >>> >>> > > >> >> >> > > > > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > > >>> >>> > > >> >> >> > > > > > > > > >>> >>> > > >> >> >> > > > > > > > >>> >>> > > >> >> >> > > > > > > >>> >>> > > >> >> >> > > > > >>> >>> > > > > >>> > > > > > > > > >
