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. > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > >> > > > > ssl.keystore.password appears a > > few > > > > > lines > > > > > > > > > above. > > > > > > > > > >> Would > > > > > > > > > >> > > > > there > > > > > > > > > >> > > > > > > be > > > > > > > > > >> > > > > > > > >> any > > > > > > > > > >> > > > > > > > >> > > issue > > > > > > > > > >> > > > > > > > >> > > > > with mixture of connections > > (with old > > > > > and > > > > > > > new > > > > > > > > > >> > > password) ? > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > >> > > > > Cheers > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > >> > > > > On Mon, Nov 20, 2017 at 5:57 AM, > > > > Rajini > > > > > > > > > Sivaram < > > > > > > > > > >> > > > > > > > >> > > rajinisiva...@gmail.com > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > >> > > > > wrote: > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > >> > > > > > Hi all, > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > I have submitted KIP-226 to > > enable > > > > > > > dynamic > > > > > > > > > >> > > > > reconfiguration > > > > > > > > > >> > > > > > > of > > > > > > > > > >> > > > > > > > >> > brokers > > > > > > > > > >> > > > > > > > >> > > > > > without restart: > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > https://cwiki.apache.org/ > > > > > > > > > >> > > confluence/display/KAFKA/KIP- > > > > > > > > > >> > > > > > > > >> > > > > > 226+-+Dynamic+Broker+ > > Configuration > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > The KIP proposes to extend the > > > > > current > > > > > > > > > dynamic > > > > > > > > > >> > > > > replication > > > > > > > > > >> > > > > > > > quota > > > > > > > > > >> > > > > > > > >> > > > > > configuration for brokers to > > > > support > > > > > > > > dynamic > > > > > > > > > >> > > > > reconfiguration > > > > > > > > > >> > > > > > > > of > > > > > > > > > >> > > > > > > > >> a > > > > > > > > > >> > > > > > > > >> > > > limited > > > > > > > > > >> > > > > > > > >> > > > > > set of configuration options > > that > > > > are > > > > > > > > > typically > > > > > > > > > >> > > updated > > > > > > > > > >> > > > > > > during > > > > > > > > > >> > > > > > > > >> the > > > > > > > > > >> > > > > > > > >> > > > > lifetime > > > > > > > > > >> > > > > > > > >> > > > > > of a broker. > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > Feedback and suggestions are > > > > welcome. > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > Thank you... > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > Regards, > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > Rajini > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > > >> > > > > > > > >> > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
