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