Hi all, I'd also like to as the community here who were participating the discussion of KIP-226 to take a look at KIP-248 (that is making kafka-configs.sh fully function with AdminClient and a Java based ConfigCommand). It would be much appreciated to get feedback on that as it plays an important role for KIP-226 and other long-waited features.
Thanks, Viktor On Wed, Jan 24, 2018 at 6:56 AM, Ismael Juma <ism...@juma.me.uk> wrote: > Hi Rajini, > > I think the proposal makes sense. One suggestion: can we just allow the > config to be passed? That is, leave out the properties config for now. > > On Tue, Jan 23, 2018 at 3:01 PM, Rajini Sivaram <rajinisiva...@gmail.com> > wrote: > > > Since we are running out of time to get the whole ConfigCommand converted > > to using the new AdminClient for 1.1.0 (KIP-248), we need a way to enable > > ConfigCommand to handle broker config updates (implemented by KIP-226). > As > > a simple first step, it would make sense to use the existing > ConfigCommand > > tool to perform broker config updates enabled by this KIP. Since config > > validation and password encryption are performed by the broker, this will > > be easier to do with the new AdminClient. To do this, we need to add > > command line options for new admin client to kafka-configs.sh. Dynamic > > broker config updates alone will be done under KIP-226 using the new > admin > > client to make this feature usable.. The new command line options > > (consistent with KIP-248) that will be added to ConfigCommand will be: > > > > - --bootstrap-server *host:port* > > - --adminclient.config *config-file* > > - --adminclient.properties *k1=v1,k2=v2* > > > > If anyone has any concerns about these options being added to > > kafka-configs.sh, please let me know. Otherwise, I will update KIP-226 > and > > add the options to one of the KIP-226 PRs. > > > > Thank you, > > > > Rajini > > > > On Wed, Jan 10, 2018 at 5:14 AM, Ismael Juma <ism...@juma.me.uk> wrote: > > > > > Thanks Rajini. Sounds good. > > > > > > Ismael > > > > > > On Wed, Jan 10, 2018 at 11:41 AM, Rajini Sivaram < > > rajinisiva...@gmail.com> > > > wrote: > > > > > > > Hi Ismael, > > > > > > > > I have updated the KIP to use AES-256 if available and AES-128 > > otherwise > > > > for password encryption. Looking at GCM, it looks like GCM is > typically > > > > used with a variable initialization vector, while we are using a > > random, > > > > but constant IV per-password. Also, AES/GCM is not supported by > Java7. > > > > Since the authentication and performance benefits of GCM are not > > required > > > > for this scenario, I am thinking I will leave the default as CBC, but > > > make > > > > sure we test GCM as well so that users have the choice. > > > > > > > > On Wed, Jan 10, 2018 at 1:01 AM, Colin McCabe <cmcc...@apache.org> > > > wrote: > > > > > > > > > Thanks, Rajini. That makes sense. > > > > > > > > > > regards, > > > > > Colin > > > > > > > > > > On Tue, Jan 9, 2018, at 14:38, Rajini Sivaram wrote: > > > > > > Hi Colin, > > > > > > > > > > > > Thank you for reviewing. > > > > > > > > > > > > Yes, validation is done on the broker, not the client. > > > > > > > > > > > > All configs from ZooKeeper are processed and any config that > could > > > not > > > > be > > > > > > applied are logged as warnings. This includes any configs that > are > > > not > > > > > > dynamic in the broker version or any configs that are not > supported > > > in > > > > > the > > > > > > broker version. If you downgrade to a version that is older than > > this > > > > KIP > > > > > > (1.0 for example), then you don't get any warnings however. > > > > > > > > > > > > > > > > > > On Tue, Jan 9, 2018 at 9:38 PM, Colin McCabe <cmcc...@apache.org > > > > > > wrote: > > > > > > > > > > > > > On Mon, Dec 18, 2017, at 13:40, Jason Gustafson wrote: > > > > > > > > Hi Rajini, > > > > > > > > > > > > > > > > Looking good. Just a few questions. > > > > > > > > > > > > > > > > 1. (Related to Jay's comment) Is the validate() method on > > > > > Reconfigurable > > > > > > > > necessary? I would have thought we'd validate using the > > > ConfigDef. > > > > > Do you > > > > > > > > have a use case in mind in which the reconfigurable component > > > only > > > > > > > permits > > > > > > > > certain reconfigurations? > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > Sorry if this is a dumb question, but when we talk about > > validating > > > > on > > > > > the > > > > > > > ConfigDef, we're talking about validating on the server side, > > > right? > > > > > The > > > > > > > software on the client side might be older or newer than the > > > software > > > > > on > > > > > > > the broker side, so it seems inadvisable to do the validation > > > there. > > > > > > > > > > > > > > Also, after a software downgrade, when the broker is restarted, > > it > > > > > might > > > > > > > find that there is a configuration key that is stored in ZK > that > > is > > > > not > > > > > > > dynamic in its (older) software version. It seems like, with > the > > > > > current > > > > > > > proposal, the broker will use the value found in the local > > > > > configuration > > > > > > > (config file) rather than the new ZK version. Should the > broker > > > > print > > > > > out > > > > > > > a WARN message in that scenario? > > > > > > > > > > > > > > best, > > > > > > > Colin > > > > > > > > > > > > > > > 2. Should Reconfigurable extend Configurable or is the > initial > > > > > > > > configuration also done through reconfigure()? I ask because > > not > > > > all > > > > > > > > plugins interfaces currently extend Configurable (e.g. > > > > > > > > KafkaPrincipalBuilder). > > > > > > > > 3. You mentioned a couple changes to DescribeConfigsOptions > and > > > > > > > > DescribeConfigsResult. Perhaps we should list the changes > > > > > explicitly? One > > > > > > > > not totally obvious case is what the synonyms() getter would > > > return > > > > > if > > > > > > > the > > > > > > > > option is not specified (i.e. should it raise an exception or > > > > return > > > > > an > > > > > > > > empty list?). > > > > > > > > 4. Config entries in the DescribeConfigs response have an > > > > is_default > > > > > > > flag. > > > > > > > > Could that be replaced with the more general config_source? > > > > > > > > 5. Bit of an internal question, but how do you handle config > > > > > > > dependencies? > > > > > > > > For example, suppose I want to add a listener and configure > its > > > > > principal > > > > > > > > builder at once. You'd have to validate the principal builder > > > > config > > > > > in > > > > > > > the > > > > > > > > context of the listener config, so I guess the order of the > > > entries > > > > > in > > > > > > > > AlterConfigs is significant? > > > > > > > > 6. KIP-48 (delegation tokens) gives us a master secret which > is > > > > > shared by > > > > > > > > all brokers. Do you think we would make this dynamically > > > > > configurable? > > > > > > > > Alternatively, it might be possible to use it to encrypt the > > > other > > > > > > > > passwords we store in zookeeper. > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Jason > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Dec 18, 2017 at 10:16 AM, Rajini Sivaram < > > > > > > > rajinisiva...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi Jay, > > > > > > > > > > > > > > > > > > Thank you for reviewing the KIP. > > > > > > > > > > > > > > > > > > 1) Yes, makes sense. I will update the PR. There are some > > > config > > > > > > > updates > > > > > > > > > that may be allowed depending on the context (e.g. some > > > security > > > > > > > configs > > > > > > > > > can be updated for new listeners, but not existing > > listeners). > > > > > Perhaps > > > > > > > it > > > > > > > > > is ok to mark them dynamic in the documentation. > AdminClient > > > > would > > > > > give > > > > > > > > > appropriate error messages if the update is not allowed. > > > > > > > > > 2) Internally, in the implementation, a mixture of direct > > > config > > > > > > > updates > > > > > > > > > (e.g log config as you have pointed out) and reconfigure > > method > > > > > > > invocations > > > > > > > > > (e.g. SslFactory) are used. For configurable plugins (e.g. > > > > metrics > > > > > > > > > reporter), we require the Reconfigurable interface to > ensure > > > that > > > > > we > > > > > > > can > > > > > > > > > validate any custom configs and avoid reconfiguration for > > > plugin > > > > > > > versions > > > > > > > > > that don't support it. > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Dec 18, 2017 at 5:49 PM, Jay Kreps < > j...@confluent.io > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Two thoughts on implementation (shouldn't effect the > KIP): > > > > > > > > > > > > > > > > > > > > 1. It might be nice to add a parameter to ConfigDef > > which > > > > says > > > > > > > > > whether a > > > > > > > > > > configuration is dynamically updatable or not so that > we > > > can > > > > > give > > > > > > > > > error > > > > > > > > > > messages if it isn't and also have it reflected in the > > > > > > > auto-generated > > > > > > > > > > docs. > > > > > > > > > > 2. For many systems they don't really need to take > > action > > > > if a > > > > > > > config > > > > > > > > > > changes, they just need to use the new value. Changing > > > them > > > > > all to > > > > > > > > > > Reconfigurable requires managing a fair amount of > > > mutability > > > > > in > > > > > > > each > > > > > > > > > > class > > > > > > > > > > that accepts changes. Some need this since they need > to > > > take > > > > > > > actions > > > > > > > > > > when a > > > > > > > > > > config changes, but it seems like many just need to > > update > > > > > some > > > > > > > value. > > > > > > > > > > For > > > > > > > > > > the later you might just be able to do something like > > what > > > > we > > > > > do > > > > > > > for > > > > > > > > > > LogConfig where there is a single CurrentConfig > instance > > > > that > > > > > has > > > > > > > a > > > > > > > > > > reference to the current KafkaConfig and always > > reference > > > > your > > > > > > > > > > configurable > > > > > > > > > > parameters via this (e.g. config.current.myConfig). > > Dunno > > > if > > > > > that > > > > > > > is > > > > > > > > > > actually better, but thought I'd throw it out there. > > > > > > > > > > > > > > > > > > > > -Jay > > > > > > > > > > > > > > > > > > > > On Sun, Dec 10, 2017 at 8:09 AM, Rajini Sivaram < > > > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > > > > > > > > > > > Thank you! > > > > > > > > > > > > > > > > > > > > > > 5. Yes, that makes sense. Agree that we don't want to > add > > > > > protocol > > > > > > > > > > changes > > > > > > > > > > > to *UpdateMetadataRequest* in this KIP. I have moved > the > > > > > update of > > > > > > > > > > > *log.message.format.version* and > *inter.broker.protocol. > > > > > version* > > > > > > > to > > > > > > > > > > reduce > > > > > > > > > > > restarts during upgrade to* Future Work*. We can do > this > > > in a > > > > > > > follow-on > > > > > > > > > > > KIP. > > > > > > > > > > > > > > > > > > > > > > I will wait for another day to see if there are any > more > > > > > comments > > > > > > > and > > > > > > > > > > start > > > > > > > > > > > vote on Tuesday if there are no other concerns. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sat, Dec 9, 2017 at 12:22 AM, Jun Rao < > > j...@confluent.io > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi, Rajini, > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the reply. They all make sense. > > > > > > > > > > > > > > > > > > > > > > > > 5. Got it. Note that currently, only live brokers are > > > > > registered > > > > > > > in > > > > > > > > > ZK. > > > > > > > > > > > > Another thing is that I am not sure that we want > every > > > > > broker to > > > > > > > read > > > > > > > > > > all > > > > > > > > > > > > broker registrations directly from ZK. It's probably > > > better > > > > > to > > > > > > > have > > > > > > > > > the > > > > > > > > > > > > controller propagate this information. That will > > require > > > > > > > changing the > > > > > > > > > > > > UpdateMetadataRequest protocol though. So, I am not > > sure > > > if > > > > > you > > > > > > > want > > > > > > > > > to > > > > > > > > > > > do > > > > > > > > > > > > that in the same KIP. > > > > > > > > > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Dec 8, 2017 at 6:07 AM, Rajini Sivaram < > > > > > > > > > > rajinisiva...@gmail.com> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > > > > > > > > > > > > > > > Thank you for the review. > > > > > > > > > > > > > > > > > > > > > > > > > > 1. No, I am hoping to migrate partitions to new > > > threads. > > > > We > > > > > > > just > > > > > > > > > need > > > > > > > > > > > to > > > > > > > > > > > > > ensure they don't run concurrently. > > > > > > > > > > > > > > > > > > > > > > > > > > 2. AdminClient has a validateOnly option for > > > > AlterConfigs. > > > > > Any > > > > > > > > > > > exceptions > > > > > > > > > > > > > or return value of false from > Reconfigurable#validate > > > > would > > > > > > > fail > > > > > > > > > the > > > > > > > > > > > > > AlterConfigsRequest. > > > > > > > > > > > > > > > > > > > > > > > > > > 3. Yes, we will support describe and alter of > configs > > > > with > > > > > > > listener > > > > > > > > > > > > prefix. > > > > > > > > > > > > > We will not allow alterConfigs() of security > configs > > > > > without > > > > > > > the > > > > > > > > > > > listener > > > > > > > > > > > > > prefix, since we need to prevent the whole cluster > > > being > > > > > made > > > > > > > > > > unusable. > > > > > > > > > > > > > > > > > > > > > > > > > > 4. Thank you, will make a note of that. > > > > > > > > > > > > > > > > > > > > > > > > > > 5. When we are upgrading (from 1.0 to 2.0 for > > example), > > > > my > > > > > > > > > > > understanding > > > > > > > > > > > > is > > > > > > > > > > > > > that we set inter.broker.protocol.version=1.0, do > a > > > > > rolling > > > > > > > > > upgrade > > > > > > > > > > of > > > > > > > > > > > > the > > > > > > > > > > > > > whole cluster and when all brokers are at 2.0, we > do > > > > > another > > > > > > > > > rolling > > > > > > > > > > > > > upgrade with inter.broker.protocol.version=2.0. > > > Jason's > > > > > > > suggestion > > > > > > > > > > was > > > > > > > > > > > > to > > > > > > > > > > > > > avoid the second rolling upgrade by enabling > dynamic > > > > > update of > > > > > > > > > > > > > inter.broker.protocol.version. To set > > > > > > > > > inter.broker.protocol.version= > > > > > > > > > > > 2.0 > > > > > > > > > > > > > dynamically, we need to verify not just that the > > > current > > > > > > > broker is > > > > > > > > > on > > > > > > > > > > > > > version 2.0, but that all brokers int the cluster > are > > > on > > > > > > > version > > > > > > > > > 2.0 > > > > > > > > > > (I > > > > > > > > > > > > > thought that was the reason for the second rolling > > > > > upgrade). > > > > > > > The > > > > > > > > > > broker > > > > > > > > > > > > > version in ZK would enable that verification before > > > > > performing > > > > > > > the > > > > > > > > > > > > update. > > > > > > > > > > > > > > > > > > > > > > > > > > 6. The config source would be > > > > STATIC_BROKER_CONFIG/DYNAMIC_ > > > > > > > > > > > > BROKER_CONFIG, > > > > > > > > > > > > > the config name would be listener.name.listenerA. > > > > configX. > > > > > And > > > > > > > > > > synonyms > > > > > > > > > > > > > list > > > > > > > > > > > > > in describeConfigs() would list > > > listener.name.listenerA. > > > > > > > configX > > > > > > > > > as > > > > > > > > > > > well > > > > > > > > > > > > > as > > > > > > > > > > > > > configX. > > > > > > > > > > > > > > > > > > > > > > > > > > 7. I think `default` is an overused terminology > > > already. > > > > > When > > > > > > > > > configs > > > > > > > > > > > are > > > > > > > > > > > > > described, they return a flag indicating if the > value > > > is > > > > a > > > > > > > default. > > > > > > > > > > And > > > > > > > > > > > > in > > > > > > > > > > > > > the broker, we have so many levels of configs > already > > > and > > > > > we > > > > > > > are > > > > > > > > > > adding > > > > > > > > > > > > so > > > > > > > > > > > > > many more, that it may be better to use a different > > > term. > > > > > It > > > > > > > > > doesn't > > > > > > > > > > > have > > > > > > > > > > > > > to be synonyms, but since we want to use the same > > term > > > > for > > > > > > > topics > > > > > > > > > and > > > > > > > > > > > > > brokers and we have listener configs which override > > > > > > > non-prefixed > > > > > > > > > > > security > > > > > > > > > > > > > configs, perhaps it is ok? > > > > > > > > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Dec 6, 2017 at 11:50 PM, Jun Rao < > > > > j...@confluent.io > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > A couple more things. > > > > > > > > > > > > > > > > > > > > > > > > > > > > 6. For the SSL/SASL configurations with the > > listener > > > > > prefix, > > > > > > > do > > > > > > > > > we > > > > > > > > > > > need > > > > > > > > > > > > > > another level in config_source since it's neither > > > topic > > > > > nor > > > > > > > > > broker? > > > > > > > > > > > > > > > > > > > > > > > > > > > > 7. For include_synonyms in DescribeConfigs, the > > name > > > > > makes > > > > > > > sense > > > > > > > > > > for > > > > > > > > > > > > the > > > > > > > > > > > > > > topic level configs. Not sure if it makes sense > for > > > > other > > > > > > > > > > > hierarchies. > > > > > > > > > > > > > > Perhaps sth more generic like default will be > > better? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Dec 6, 2017 at 3:41 PM, Jun Rao < > > > > > j...@confluent.io> > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, Rajini, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the kip. Looks good overall. A few > > > > comments > > > > > > > below. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1. "num.replica.fetchers: Affinity of > partitions > > to > > > > > threads > > > > > > > > > will > > > > > > > > > > be > > > > > > > > > > > > > > > preserved for ordering." Does that mean the new > > > > fetcher > > > > > > > threads > > > > > > > > > > > won't > > > > > > > > > > > > > be > > > > > > > > > > > > > > > used until new partitions are added? This may > be > > > > > limiting. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. I am wondering how the result from > > > > > > > > > > reporter.validate(Map<String, > > > > > > > > > > > > ?> > > > > > > > > > > > > > > > configs) will be used. If it returns false, do > we > > > > > return > > > > > > > false > > > > > > > > > to > > > > > > > > > > > the > > > > > > > > > > > > > > admin > > > > > > > > > > > > > > > client for the validation call, which will > seem a > > > bit > > > > > > > weird? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 3. For the SSL and SASL configuration changes, > do > > > we > > > > > > > support > > > > > > > > > > those > > > > > > > > > > > > with > > > > > > > > > > > > > > > the listener prefix (e.g., > > > external-ssl-lisener.ssl. > > > > > > > > > > > > > keystore.location). > > > > > > > > > > > > > > > If so, do we plan to include them in the result > > of > > > > > > > > > > > describeConfigs()? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 4. "Updates to advertised.listeners will > > > re-register > > > > > the > > > > > > > new > > > > > > > > > > > listener > > > > > > > > > > > > > in > > > > > > > > > > > > > > > ZK". To support this, we will likely need > > > additional > > > > > logic > > > > > > > in > > > > > > > > > the > > > > > > > > > > > > > > > controller such that the controller can > broadcast > > > the > > > > > > > metadata > > > > > > > > > > with > > > > > > > > > > > > the > > > > > > > > > > > > > > new > > > > > > > > > > > > > > > listeners to every broker. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 5. Including broker version in broker > > registration > > > in > > > > > ZK. > > > > > > > I am > > > > > > > > > > not > > > > > > > > > > > > sure > > > > > > > > > > > > > > > the usage of that. Each broker knows its > version. > > > So, > > > > > is > > > > > > > that > > > > > > > > > for > > > > > > > > > > > the > > > > > > > > > > > > > > > controller? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Dec 5, 2017 at 11:05 AM, Colin McCabe < > > > > > > > > > > cmcc...@apache.org> > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >> On Tue, Dec 5, 2017, at 06:01, Rajini Sivaram > > > wrote: > > > > > > > > > > > > > > >> > Hi Colin, > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > KAFKA-5722 already has an owner, so I didn't > > > want > > > > to > > > > > > > confuse > > > > > > > > > > the > > > > > > > > > > > > two > > > > > > > > > > > > > > >> > KIPs. They can be done independently of > each > > > > > other. The > > > > > > > > > goal > > > > > > > > > > is > > > > > > > > > > > > to > > > > > > > > > > > > > > try > > > > > > > > > > > > > > >> and > > > > > > > > > > > > > > >> > validate every config to the minimum > > validation > > > > > already > > > > > > > in > > > > > > > > > the > > > > > > > > > > > > > broker > > > > > > > > > > > > > > >> for > > > > > > > > > > > > > > >> > the static configs, but in some cases to a > > more > > > > > > > restrictive > > > > > > > > > > > level. > > > > > > > > > > > > > So > > > > > > > > > > > > > > a > > > > > > > > > > > > > > >> > typo like a file-not-found or > class-not-found > > > > would > > > > > > > > > definitely > > > > > > > > > > > > fail > > > > > > > > > > > > > > the > > > > > > > > > > > > > > >> > AlterConfigs request (validation is > performed > > by > > > > > > > > > AlterConfigs > > > > > > > > > > > > > > regardless > > > > > > > > > > > > > > >> > of validateOnly flag). I am working out the > > > > > additional > > > > > > > > > > > validation > > > > > > > > > > > > I > > > > > > > > > > > > > > can > > > > > > > > > > > > > > >> > perform as I implement updates for each > > config. > > > > For > > > > > > > example, > > > > > > > > > > > > > > >> > inter-broker keystore update will not be > > allowed > > > > > unless > > > > > > > it > > > > > > > > > can > > > > > > > > > > > be > > > > > > > > > > > > > > >> > verified against the currently configured > > > > > truststore. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> HI Rajini, > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> I agree. It's probably better to avoid > > expanding > > > > the > > > > > > > scope of > > > > > > > > > > > > > KIP-226. > > > > > > > > > > > > > > >> I hope we can get to KAFKA-5722 soon, though, > > > since > > > > it > > > > > > > will > > > > > > > > > > really > > > > > > > > > > > > > > >> improve the user experience for this feature. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> regards, > > > > > > > > > > > > > > >> Colin > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > On Sat, Dec 2, 2017 at 10:15 PM, Colin > McCabe > > < > > > > > > > > > > > cmcc...@apache.org > > > > > > > > > > > > > > > > > > > > > > > > > > > >> wrote: > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > On Tue, Nov 28, 2017, at 14:48, Rajini > > Sivaram > > > > > wrote: > > > > > > > > > > > > > > >> > > > Hi Colin, > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > Thank you for reviewing the KIP. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > *kaka-configs.sh* will be converted to > use > > > > > > > *AdminClient* > > > > > > > > > > > under > > > > > > > > > > > > > > >> > > > KAFKA-5722. > > > > > > > > > > > > > > >> > > > This is targeted for the next release > > > (1.1.0). > > > > > Under > > > > > > > > > this > > > > > > > > > > > KIP, > > > > > > > > > > > > > we > > > > > > > > > > > > > > >> will > > > > > > > > > > > > > > >> > > > implement *AdminClient#alterConfigs* for > > the > > > > > dynamic > > > > > > > > > > configs > > > > > > > > > > > > > > listed > > > > > > > > > > > > > > >> in > > > > > > > > > > > > > > >> > > > the KIP. This will include validation of > > the > > > > > > > configs and > > > > > > > > > > > will > > > > > > > > > > > > > > return > > > > > > > > > > > > > > >> > > > appropriate errors if configs are > invalid. > > > > > > > Integration > > > > > > > > > > tests > > > > > > > > > > > > > will > > > > > > > > > > > > > > >> also be > > > > > > > > > > > > > > >> > > > added using AdmnClient. Only the actual > > > > > conversion > > > > > > > of > > > > > > > > > > > > > > ConfigCommand > > > > > > > > > > > > > > >> to > > > > > > > > > > > > > > >> > > > use AdminClient will be left to be done > > > under > > > > > > > > > KAFKA-5722. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > Hi Rajini, > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > It seems like there is no KIP yet for > > > > KAFKA-5722. > > > > > > > Does it > > > > > > > > > > > make > > > > > > > > > > > > > > sense > > > > > > > > > > > > > > >> to > > > > > > > > > > > > > > >> > > describe the conversion of > kafka-configs.sh > > to > > > > use > > > > > > > > > > AdminClient > > > > > > > > > > > > in > > > > > > > > > > > > > > >> > > KIP-226? Since the AlterConfigs RPCs > > already > > > > > exist, > > > > > > > it > > > > > > > > > > should > > > > > > > > > > > > be > > > > > > > > > > > > > > >> pretty > > > > > > > > > > > > > > >> > > straightforward. This would also let us > add > > > > some > > > > > > > > > > information > > > > > > > > > > > > > about > > > > > > > > > > > > > > >> how > > > > > > > > > > > > > > >> > > errors will be handled, which is pretty > > > > important > > > > > for > > > > > > > > > users. > > > > > > > > > > > > For > > > > > > > > > > > > > > >> > > example, will kafka-configs.sh give an > error > > > if > > > > > the > > > > > > > user > > > > > > > > > > > makes a > > > > > > > > > > > > > > typo > > > > > > > > > > > > > > >> > > when setting a configuration? > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > Once KAFKA-5722 is implemented,* > > > > > kafka-confgs.sh* > > > > > > > can be > > > > > > > > > > > used > > > > > > > > > > > > to > > > > > > > > > > > > > > >> obtain > > > > > > > > > > > > > > >> > > > the current configuration, which can be > > > > > redirected > > > > > > > to a > > > > > > > > > > text > > > > > > > > > > > > > file > > > > > > > > > > > > > > to > > > > > > > > > > > > > > >> > > create a > > > > > > > > > > > > > > >> > > > static *server.properties* file. This > > should > > > > > help > > > > > > > when > > > > > > > > > > > > > downgrading > > > > > > > > > > > > > > >> - but > > > > > > > > > > > > > > >> > > > it does require brokers to be running. > We > > > can > > > > > also > > > > > > > > > > document > > > > > > > > > > > > how > > > > > > > > > > > > > to > > > > > > > > > > > > > > >> obtain > > > > > > > > > > > > > > >> > > > the properties using > *zookeeper-shell.sh* > > > > while > > > > > > > > > > downgrading > > > > > > > > > > > if > > > > > > > > > > > > > > >> brokers > > > > > > > > > > > > > > >> > > are > > > > > > > > > > > > > > >> > > > down. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > If we rename properties, we should add > the > > > new > > > > > > > property > > > > > > > > > to > > > > > > > > > > > ZK > > > > > > > > > > > > > > based > > > > > > > > > > > > > > >> on > > > > > > > > > > > > > > >> > > > the value of the old property when the > > > > upgraded > > > > > > > broker > > > > > > > > > > > starts > > > > > > > > > > > > > up. > > > > > > > > > > > > > > >> But we > > > > > > > > > > > > > > >> > > > would probably leave the old property as > > is. > > > > > The old > > > > > > > > > > > property > > > > > > > > > > > > > will > > > > > > > > > > > > > > >> be > > > > > > > > > > > > > > >> > > returned > > > > > > > > > > > > > > >> > > > and used as a synonym only as long as > the > > > > > broker is > > > > > > > on a > > > > > > > > > > > > version > > > > > > > > > > > > > > >> where it > > > > > > > > > > > > > > >> > > > is still valid. But it can remain in ZK > > and > > > be > > > > > > > updated > > > > > > > > > if > > > > > > > > > > > > > > >> downgrading - > > > > > > > > > > > > > > >> > > > it will be up to the user to update the > > old > > > > > > > property if > > > > > > > > > > > > > > downgrading > > > > > > > > > > > > > > >> or > > > > > > > > > > > > > > >> > > > delete it if not needed. Renaming > > properties > > > > is > > > > > > > likely > > > > > > > > > to > > > > > > > > > > be > > > > > > > > > > > > > > >> confusing > > > > > > > > > > > > > > >> > > in any > > > > > > > > > > > > > > >> > > > case even without dynamic configs, so > > > > hopefully > > > > > it > > > > > > > will > > > > > > > > > be > > > > > > > > > > > > very > > > > > > > > > > > > > > >> rare. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > Sounds good. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > best, > > > > > > > > > > > > > > >> > > Colin > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > Rajini > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > On Tue, Nov 28, 2017 at 7:47 PM, Colin > > > McCabe > > > > < > > > > > > > > > > > > > cmcc...@apache.org > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >> > > wrote: > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > Hi Rajini, > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > This seems like a nice improvement! > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > One thing that is a bit concerning is > > > that, > > > > if > > > > > > > > > > > > > > >> bin/kafka-configs.sh is > > > > > > > > > > > > > > >> > > > > used, there is no way for the broker > to > > > > give > > > > > > > feedback > > > > > > > > > > or > > > > > > > > > > > > > error > > > > > > > > > > > > > > >> > > > > messages. The broker can't say > "sorry, > > I > > > > > can't > > > > > > > > > > > reconfigure > > > > > > > > > > > > > that > > > > > > > > > > > > > > >> in > > > > > > > > > > > > > > >> > > that > > > > > > > > > > > > > > >> > > > > way." Or even "that configuration > > > property > > > > > is not > > > > > > > > > > > > > > reconfigurable > > > > > > > > > > > > > > >> in > > > > > > > > > > > > > > >> > > > > this version of the software." It > seems > > > > like > > > > > in > > > > > > > the > > > > > > > > > > > > examples > > > > > > > > > > > > > > >> give, > > > > > > > > > > > > > > >> > > > > users will simply set a configuration > > > using > > > > > > > > > > > > > > bin/kafka-configs.sh, > > > > > > > > > > > > > > >> but > > > > > > > > > > > > > > >> > > > > then they have to check the broker log > > > files > > > > > to > > > > > > > see if > > > > > > > > > > it > > > > > > > > > > > > > could > > > > > > > > > > > > > > >> > > actually > > > > > > > > > > > > > > >> > > > > be applied. And even if it couldn't > be > > > > > applied, > > > > > > > then > > > > > > > > > it > > > > > > > > > > > > still > > > > > > > > > > > > > > >> lingers > > > > > > > > > > > > > > >> > > > > in ZooKeeper. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > This seems like it would lead to a lot > > of > > > > user > > > > > > > > > > confusion, > > > > > > > > > > > > > since > > > > > > > > > > > > > > >> they > > > > > > > > > > > > > > >> > > get > > > > > > > > > > > > > > >> > > > > no feedback when reconfiguring > > something. > > > > For > > > > > > > > > example, > > > > > > > > > > > > there > > > > > > > > > > > > > > >> will be a > > > > > > > > > > > > > > >> > > > > lot of scenarios where someone finds a > > > > > > > reconfiguration > > > > > > > > > > > > command > > > > > > > > > > > > > > on > > > > > > > > > > > > > > >> > > > > Google, runs it, but then it doesn't > do > > > > > anything > > > > > > > > > because > > > > > > > > > > > the > > > > > > > > > > > > > > >> software > > > > > > > > > > > > > > >> > > > > version is different. And there's no > > > error > > > > > > > message or > > > > > > > > > > > > > feedback > > > > > > > > > > > > > > >> about > > > > > > > > > > > > > > >> > > > > this. It just doesn't work. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > To prevent this, I think we should > > convert > > > > > > > > > > > > > bin/kafka-configs.sh > > > > > > > > > > > > > > >> to use > > > > > > > > > > > > > > >> > > > > AdminClient's AlterConfigsRequest. > This > > > > > allows > > > > > > > us to > > > > > > > > > > > detect > > > > > > > > > > > > > > >> scenarios > > > > > > > > > > > > > > >> > > > > where, because of a typo, different > > > software > > > > > > > version, > > > > > > > > > > or a > > > > > > > > > > > > > value > > > > > > > > > > > > > > >> of the > > > > > > > > > > > > > > >> > > > > wrong type (eg. string vs. int), the > > given > > > > > > > > > configuration > > > > > > > > > > > > > cannot > > > > > > > > > > > > > > be > > > > > > > > > > > > > > >> > > > > applied. We really should convert > > > > > > > kafka-configs.sh to > > > > > > > > > > use > > > > > > > > > > > > > > >> AdminClient > > > > > > > > > > > > > > >> > > > > anyway, for all the usual reasons-- > > people > > > > > want to > > > > > > > > > lock > > > > > > > > > > > down > > > > > > > > > > > > > > >> ZooKeeper, > > > > > > > > > > > > > > >> > > > > ACLs should be enforced, internal > > > > > representations > > > > > > > > > should > > > > > > > > > > > be > > > > > > > > > > > > > > >> hidden, we > > > > > > > > > > > > > > >> > > > > should support environments where ZK > is > > > not > > > > > > > exposed, > > > > > > > > > > etc. > > > > > > > > > > > > etc. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > Another issue that I see here is, how > > does > > > > > this > > > > > > > > > interact > > > > > > > > > > > > with > > > > > > > > > > > > > > >> > > downgrade? > > > > > > > > > > > > > > >> > > > > Presumably, if the user downgrades > to a > > > > > version > > > > > > > that > > > > > > > > > > > > doesn't > > > > > > > > > > > > > > >> support > > > > > > > > > > > > > > >> > > > > KIP-226, all the dynamic > configurations > > > > > stored in > > > > > > > ZK > > > > > > > > > > > revert > > > > > > > > > > > > to > > > > > > > > > > > > > > >> whatever > > > > > > > > > > > > > > >> > > > > value they have in the local config > > files. > > > > > Do we > > > > > > > need > > > > > > > > > > to > > > > > > > > > > > > > have a > > > > > > > > > > > > > > >> > > utility > > > > > > > > > > > > > > >> > > > > that can reify the actual applied > > > > > configuration > > > > > > > into a > > > > > > > > > > > local > > > > > > > > > > > > > > text > > > > > > > > > > > > > > >> file, > > > > > > > > > > > > > > >> > > > > to make downgrades less painful? > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > With regard to upgrades, what happens > if > > > we > > > > > > > change the > > > > > > > > > > > name > > > > > > > > > > > > > of a > > > > > > > > > > > > > > >> > > > > configuration key in the future? For > > > > > example, if > > > > > > > we > > > > > > > > > > > decide > > > > > > > > > > > > to > > > > > > > > > > > > > > >> rename > > > > > > > > > > > > > > >> > > > > min.insync.replicas to > > > min.in.sync.replicas, > > > > > > > > > presumably > > > > > > > > > > we > > > > > > > > > > > > > will > > > > > > > > > > > > > > >> > > > > deprecate the old key name. And then > > > > perhaps > > > > > it > > > > > > > will > > > > > > > > > be > > > > > > > > > > > > > removed > > > > > > > > > > > > > > >> in a > > > > > > > > > > > > > > >> > > > > future release, such as Apache Kafka > > 2.0. > > > > In > > > > > this > > > > > > > > > > > scenario, > > > > > > > > > > > > > > >> should the > > > > > > > > > > > > > > >> > > > > Kafka upgrade process change the name > of > > > the > > > > > > > > > > configuration > > > > > > > > > > > > key > > > > > > > > > > > > > > in > > > > > > > > > > > > > > >> ZK > > > > > > > > > > > > > > >> > > > > from min.insync.replicas to > > > > > min.in.sync.replicas? > > > > > > > > > > > Obviously > > > > > > > > > > > > > > this > > > > > > > > > > > > > > >> will > > > > > > > > > > > > > > >> > > > > make downgrades harder, if so. But if > > it > > > > > doesn't, > > > > > > > > > then > > > > > > > > > > > > > removing > > > > > > > > > > > > > > >> > > > > deprecated configuration key synonyms > > > might > > > > > become > > > > > > > > > very > > > > > > > > > > > > > > difficult. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > best, > > > > > > > > > > > > > > >> > > > > Colin > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > On Wed, Nov 22, 2017, at 12:52, Rajini > > > > Sivaram > > > > > > > wrote: > > > > > > > > > > > > > > >> > > > > > Hi Tom, > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > >> > > > > > No, I am not proposing this as a way > > to > > > > > > > configure > > > > > > > > > > > > > replication > > > > > > > > > > > > > > >> quotas. > > > > > > > > > > > > > > >> > > > > > When > > > > > > > > > > > > > > >> > > > > > you describe broker configs using > > > > > AdminClient, > > > > > > > you > > > > > > > > > > will > > > > > > > > > > > > see > > > > > > > > > > > > > > all > > > > > > > > > > > > > > >> the > > > > > > > > > > > > > > >> > > > > > configs > > > > > > > > > > > > > > >> > > > > > persisted in > > /configs/brokers/<brokerId> > > > > in > > > > > > > > > ZooKeeper > > > > > > > > > > > and > > > > > > > > > > > > > this > > > > > > > > > > > > > > >> > > includes > > > > > > > > > > > > > > >> > > > > > leader.replication.throttled.rate, > > > > > > > > > > > follower.replication. > > > > > > > > > > > > > > >> > > throttled.rate > > > > > > > > > > > > > > >> > > > > > etc. But > > > > > > > > > > > > > > >> > > > > > the broker configs that can be > altered > > > > using > > > > > > > > > > AdminClient > > > > > > > > > > > > as > > > > > > > > > > > > > a > > > > > > > > > > > > > > >> result > > > > > > > > > > > > > > >> > > of > > > > > > > > > > > > > > >> > > > > > this KIP are those explicitly stated > > in > > > > the > > > > > KIP > > > > > > > > > (does > > > > > > > > > > > not > > > > > > > > > > > > > > >> include > > > > > > > > > > > > > > >> > > any of > > > > > > > > > > > > > > >> > > > > > the quota configs). > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > >> > > > > > Regards, > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > >> > > > > > Rajini > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > >> > > > > > On Wed, Nov 22, 2017 at 7:54 PM, Tom > > > > > Bentley < > > > > > > > > > > > > > > >> t.j.bent...@gmail.com> > > > > > > > > > > > > > > >> > > > > > wrote: > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > >> > > > > > > Hi Rajini, > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > Just to clarify, are you proposing > > > this > > > > > as a > > > > > > > way > > > > > > > > > to > > > > > > > > > > > > > > configure > > > > > > > > > > > > > > >> > > > > interbroker > > > > > > > > > > > > > > >> > > > > > > throttling/quotas? I don't think > you > > > > are, > > > > > just > > > > > > > > > > wanted > > > > > > > > > > > to > > > > > > > > > > > > > > check > > > > > > > > > > > > > > >> > > (since > > > > > > > > > > > > > > >> > > > > > > KIP-179 proposes a different > > mechanism > > > > for > > > > > > > setting > > > > > > > > > > > them > > > > > > > > > > > > > > which > > > > > > > > > > > > > > >> > > supports > > > > > > > > > > > > > > >> > > > > > > their automatic removal). > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > Cheers, > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > Tom > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > On 22 November 2017 at 18:28, > Rajini > > > > > Sivaram < > > > > > > > > > > > > > > >> > > rajinisiva...@gmail.com> > > > > > > > > > > > > > > >> > > > > > > wrote: > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > >> > > > > > > > I have made an update to the KIP > > to > > > > > > > optionally > > > > > > > > > > > return > > > > > > > > > > > > > all > > > > > > > > > > > > > > >> the > > > > > > > > > > > > > > >> > > config > > > > > > > > > > > > > > >> > > > > > > > synonyms in > > > *DescribeConfigsResponse*. > > > > > The > > > > > > > > > > synonyms > > > > > > > > > > > > are > > > > > > > > > > > > > > >> returned > > > > > > > > > > > > > > >> > > in > > > > > > > > > > > > > > >> > > > > the > > > > > > > > > > > > > > >> > > > > > > > order of precedence. > > > > > AlterConfigsResponse > > > > > > > will > > > > > > > > > not > > > > > > > > > > > be > > > > > > > > > > > > > > >> modified > > > > > > > > > > > > > > >> > > by the > > > > > > > > > > > > > > >> > > > > > > KIP. > > > > > > > > > > > > > > >> > > > > > > > Since many configs already have > > > > various > > > > > > > > > overrides > > > > > > > > > > > > (e.g. > > > > > > > > > > > > > > >> topic > > > > > > > > > > > > > > >> > > configs > > > > > > > > > > > > > > >> > > > > > > with > > > > > > > > > > > > > > >> > > > > > > > broker overrides, security > > > properties > > > > > with > > > > > > > > > > listener > > > > > > > > > > > > name > > > > > > > > > > > > > > >> > > overrides) > > > > > > > > > > > > > > >> > > > > and > > > > > > > > > > > > > > >> > > > > > > we > > > > > > > > > > > > > > >> > > > > > > > will be adding more levels with > > > > dynamic > > > > > > > configs, > > > > > > > > > > it > > > > > > > > > > > > will > > > > > > > > > > > > > > be > > > > > > > > > > > > > > >> > > useful to > > > > > > > > > > > > > > >> > > > > > > > obtain the full list in order of > > > > > precedence. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > On Tue, Nov 21, 2017 at 11:24 > AM, > > > > Rajini > > > > > > > > > Sivaram < > > > > > > > > > > > > > > >> > > > > > > rajinisiva...@gmail.com> > > > > > > > > > > > > > > >> > > > > > > > wrote: > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > Hi Ted, > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > You can quote the config name, > > but > > > > it > > > > > is > > > > > > > not > > > > > > > > > > > > necessary > > > > > > > > > > > > > > for > > > > > > > > > > > > > > >> > > > > deleting a > > > > > > > > > > > > > > >> > > > > > > > > config since the name doesn't > > > > contain > > > > > any > > > > > > > > > > special > > > > > > > > > > > > > > >> characters > > > > > > > > > > > > > > >> > > that > > > > > > > > > > > > > > >> > > > > > > > requires > > > > > > > > > > > > > > >> > > > > > > > > quoting. > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > > On Mon, Nov 20, 2017 at 9:20 > PM, > > > Ted > > > > > Yu < > > > > > > > > > > > > > > >> yuzhih...@gmail.com> > > > > > > > > > > > > > > >> > > > > wrote: > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > >> > > > > > > > >> Thanks for the quick > response. > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> It seems the config following > > > > > > > --delete-config > > > > > > > > > > > > should > > > > > > > > > > > > > be > > > > > > > > > > > > > > >> > > quoted. > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> Cheers > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> On Mon, Nov 20, 2017 at 12:02 > > PM, > > > > > Rajini > > > > > > > > > > Sivaram > > > > > > > > > > > < > > > > > > > > > > > > > > >> > > > > > > > rajinisiva...@gmail.com > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > >> wrote: > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> > Ted, > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > >> > Have added an example for > > > > > > > --delete-config. > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > >> > On Mon, Nov 20, 2017 at > 7:42 > > > PM, > > > > > Ted > > > > > > > Yu < > > > > > > > > > > > > > > >> > > yuzhih...@gmail.com> > > > > > > > > > > > > > > >> > > > > > > wrote: > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > >> > > bq. There is a > > > --delete-config > > > > > option > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > >> > > Consider adding a sample > > with > > > > the > > > > > > > above > > > > > > > > > > > option > > > > > > > > > > > > to > > > > > > > > > > > > > > >> the KIP. > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > >> > > Thanks > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > >> > > On Mon, Nov 20, 2017 at > > 11:36 > > > > AM, > > > > > > > Rajini > > > > > > > > > > > > Sivaram > > > > > > > > > > > > > < > > > > > > > > > > > > > > >> > > > > > > > >> > rajinisiva...@gmail.com> > > > > > > > > > > > > > > >> > > > > > > > >> > > wrote: > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > Hi Ted, > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > Thank you for reviewing > > the > > > > > KIP. > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > *Would decreasing > > > network/IO > > > > > > > threads be > > > > > > > > > > > > > supported > > > > > > > > > > > > > > >> ?* > > > > > > > > > > > > > > >> > > > > > > > >> > > > Yes, As described in > the > > > KIP, > > > > > some > > > > > > > > > > > > connections > > > > > > > > > > > > > > >> will be > > > > > > > > > > > > > > >> > > > > closed if > > > > > > > > > > > > > > >> > > > > > > > >> > network > > > > > > > > > > > > > > >> > > > > > > > >> > > > thread count is reduced > > > (and > > > > > > > > > > reconnections > > > > > > > > > > > > will > > > > > > > > > > > > > > be > > > > > > > > > > > > > > >> > > > > processed on > > > > > > > > > > > > > > >> > > > > > > > >> > remaining > > > > > > > > > > > > > > >> > > > > > > > >> > > > threads) > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > *What if some keys in > > > configs > > > > > are > > > > > > > not > > > > > > > > > in > > > > > > > > > > > the > > > > > > > > > > > > > Set > > > > > > > > > > > > > > >> > > returned > > > > > > > > > > > > > > >> > > > > > > > >> > > > by > > reconfigurableConfigs()? > > > > > Would > > > > > > > > > > exception > > > > > > > > > > > > be > > > > > > > > > > > > > > >> thrown ?* > > > > > > > > > > > > > > >> > > > > > > > >> > > > No, > > > *reconfigurableConfigs() > > > > > *will > > > > > > > be > > > > > > > > > > used > > > > > > > > > > > to > > > > > > > > > > > > > > >> decide > > > > > > > > > > > > > > >> > > which > > > > > > > > > > > > > > >> > > > > > > classes > > > > > > > > > > > > > > >> > > > > > > > >> are > > > > > > > > > > > > > > >> > > > > > > > >> > > > notified when a > > > configuration > > > > > > > update is > > > > > > > > > > > > made*. > > > > > > > > > > > > > > >> > > > > > > > >> > **reconfigure(Map<String, > > > > > > > > > > > > > > >> > > > > > > > >> > > ?> > > > > > > > > > > > > > > >> > > > > > > > >> > > > configs)* will be > invoked > > > > with > > > > > all > > > > > > > of > > > > > > > > > the > > > > > > > > > > > > > > >> configured > > > > > > > > > > > > > > >> > > > > configs of > > > > > > > > > > > > > > >> > > > > > > > the > > > > > > > > > > > > > > >> > > > > > > > >> > > broker, > > > > > > > > > > > > > > >> > > > > > > > >> > > > similar to > > > > > > > *configure(Map<String, ?> > > > > > > > > > > > > > configs). > > > > > > > > > > > > > > >> *For > > > > > > > > > > > > > > >> > > > > example, > > > > > > > > > > > > > > >> > > > > > > > when > > > > > > > > > > > > > > >> > > > > > > > >> > > > *SslChannelBuilder* is > > made > > > > > > > > > > reconfigurable, > > > > > > > > > > > > it > > > > > > > > > > > > > > >> could > > > > > > > > > > > > > > >> > > just > > > > > > > > > > > > > > >> > > > > > > create a > > > > > > > > > > > > > > >> > > > > > > > >> new > > > > > > > > > > > > > > >> > > > > > > > >> > > > SslFactory with the > > latest > > > > > configs, > > > > > > > > > using > > > > > > > > > > > the > > > > > > > > > > > > > > same > > > > > > > > > > > > > > >> code > > > > > > > > > > > > > > >> > > as > > > > > > > > > > > > > > >> > > > > > > > >> > *configure()*. > > > > > > > > > > > > > > >> > > > > > > > >> > > > We avoid reconfiguring > > > > > > > > > *SslChannelBuilder > > > > > > > > > > > > > > >> > > *unnecessarily*, > > > > > > > > > > > > > > >> > > > > *for > > > > > > > > > > > > > > >> > > > > > > > >> example > > > > > > > > > > > > > > >> > > > > > > > >> > > if > > > > > > > > > > > > > > >> > > > > > > > >> > > > a topic config has > > changed, > > > > > since > > > > > > > topic > > > > > > > > > > > > configs > > > > > > > > > > > > > > >> are not > > > > > > > > > > > > > > >> > > > > listed > > > > > > > > > > > > > > >> > > > > > > in > > > > > > > > > > > > > > >> > > > > > > > >> the > > > > > > > > > > > > > > >> > > > > > > > >> > > > *SslChannelBuilder#** > > > > > > > > > > > > reconfigurableConfigs().* > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > *The sample commands > for > > > > > > > > > > bin/kafka-configs > > > > > > > > > > > > > > include > > > > > > > > > > > > > > >> > > > > > > '--add-config'. > > > > > > > > > > > > > > >> > > > > > > > >> > Would > > > > > > > > > > > > > > >> > > > > > > > >> > > > there be > > '--remove-config' > > > ?* > > > > > > > > > > > > > > >> > > > > > > > >> > > > bin/kafka-configs.sh is > > an > > > > > existing > > > > > > > > > tool > > > > > > > > > > > > whose > > > > > > > > > > > > > > >> > > parameters > > > > > > > > > > > > > > >> > > > > will > > > > > > > > > > > > > > >> > > > > > > not > > > > > > > > > > > > > > >> > > > > > > > >> be > > > > > > > > > > > > > > >> > > > > > > > >> > > > modified by this KIP. > > There > > > > is > > > > > a > > > > > > > > > > > > > --delete-config > > > > > > > > > > > > > > >> option. > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > *ssl.keystore.password > > > > appears > > > > > a > > > > > > > few > > > > > > > > > > lines > > > > > > > > > > > > > above. > > > > > > > > > > > > > > >> Would > > > > > > > > > > > > > > >> > > > > there be > > > > > > > > > > > > > > >> > > > > > > > any > > > > > > > > > > > > > > >> > > > > > > > >> > > > issue with mixture of > > > > > connections > > > > > > > (with > > > > > > > > > > old > > > > > > > > > > > > and > > > > > > > > > > > > > > new > > > > > > > > > > > > > > >> > > > > password) ?* > > > > > > > > > > > > > > >> > > > > > > > >> > > > No, passwords (and the > > > actual > > > > > > > keystore) > > > > > > > > > > are > > > > > > > > > > > > > only > > > > > > > > > > > > > > >> used > > > > > > > > > > > > > > >> > > during > > > > > > > > > > > > > > >> > > > > > > > >> > > > authentication. Any > > channel > > > > > created > > > > > > > > > using > > > > > > > > > > > the > > > > > > > > > > > > > old > > > > > > > > > > > > > > >> > > SslFactory > > > > > > > > > > > > > > >> > > > > > > will > > > > > > > > > > > > > > >> > > > > > > > >> not > > > > > > > > > > > > > > >> > > > > > > > >> > be > > > > > > > > > > > > > > >> > > > > > > > >> > > > impacted. > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > Regards, > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > Rajini > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > On Mon, Nov 20, 2017 at > > > 4:39 > > > > > PM, > > > > > > > Ted > > > > > > > > > Yu < > > > > > > > > > > > > > > >> > > > > yuzhih...@gmail.com> > > > > > > > > > > > > > > >> > > > > > > > >> wrote: > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > > bq. (e.g. increase > > > > network/IO > > > > > > > > > threads) > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > > Would decreasing > > > network/IO > > > > > > > threads > > > > > > > > > be > > > > > > > > > > > > > > supported > > > > > > > > > > > > > > >> ? > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > > bq. void > > > > > > > reconfigure(Map<String, > > > > > > > > > ?> > > > > > > > > > > > > > > configs); > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > > What if some keys in > > > > configs > > > > > are > > > > > > > not > > > > > > > > > in > > > > > > > > > > > the > > > > > > > > > > > > > Set > > > > > > > > > > > > > > >> > > returned > > > > > > > > > > > > > > >> > > > > by > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > reconfigurableConfigs() > > > > > > > > > > > > > > >> > > > > > > > >> > > > > ? Would exception be > > > > thrown ? > > > > > > > > > > > > > > >> > > > > > > > >> > > > > If so, please specify > > > which > > > > > > > exception > > > > > > > > > > > would > > > > > > > > > > > > > be > > > > > > > > > > > > > > >> thrown. > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > > The sample commands > for > > > > > > > > > > bin/kafka-configs > > > > > > > > > > > > > > include > > > > > > > > > > > > > > >> > > > > > > > '--add-config'. > > > > > > > > > > > > > > >> > > > > > > > >> > > > > Would there be > > > > > '--remove-config' > > > > > > > ? > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > > >> > > > > bq. Existing > > connections > > > > will > > > > > > > not be > > > > > > > > > > > > > affected, > > > > > > > > > > > > > > >> new > > > > > > > > > > > > > > >> > > > > connections > > > > > > > > > > > > > > >> > > > > > > > >> will > > > > > > > > > > > > > > >> > > > > > > > >> > use > > > > > > > > > > > > > > >> > > > > > > > >> > > > the > > > > > > > > > > > > > > >> > > > > > > > >> > > > > new keystore. > > > > > > > > > > > > > > >> > > > > > > > > > ... > > [Message clipped] View entire message > <?ui=2&ik=aa3c277f2b&view=lg&msg=16126bcb5fdb6a0c> >
