Made one change to the KIP - I added a *validate* method to the *Reconfigurable* interface so that we can validate new configs before they are applied.
A couple of initial implementations: 1. Dynamic updates of keystores: https://github.com/rajinisivaram/kafka/commit/3071b686973a59a45546e9db6bdb05578d2edc0b 2. Metrics reporter reconfiguration: https://github.com/rajinisivaram/kafka/commit/7c0aa1ea1d81273fe6c082d47fff16208885d3df On Fri, Dec 1, 2017 at 4:04 PM, Rajini Sivaram <rajinisiva...@gmail.com> wrote: > Hi all, > > If there are no other suggestions or comments, I will start vote next > week. In the meantime, please feel free to review and add any suggestions > or improvements. > > Regards, > > Rajini > > On Wed, Nov 29, 2017 at 11:52 AM, Rajini Sivaram <rajinisiva...@gmail.com> > wrote: > >> Hi Jason, >> >> Thanks for reviewing the KIP. >> >> I hadn't included *inter.broker.protocol.version*, but you have provided >> a good reason to do that in order to avoid an additional rolling restart >> during upgrade. I had included *log.message.format.version* along with >> other default topic configs, but it probably makes sense to do these two >> together. >> >> >> On Wed, Nov 29, 2017 at 12:00 AM, Jason Gustafson <ja...@confluent.io> >> wrote: >> >>> Hi Rajini, >>> >>> One quick question I was wondering about is whether this could be used to >>> update the inter-broker protocol version or the message format version? >>> Potentially then we'd only need one rolling restart to upgrade the >>> cluster. >>> I glanced quickly through the uses of this config in the code and it >>> seems >>> like it might be possible. Not sure if there are any complications you or >>> others can think of. >>> >>> Thanks, >>> Jason >>> >>> On Tue, Nov 28, 2017 at 2:48 PM, Rajini Sivaram <rajinisiva...@gmail.com >>> > >>> 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. >>> > >>> > 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. >>> > >>> > >>> > 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 >>> > > > > > >> > > > > > >>> > > > > > >> > > > > >>> > > > > > >> > > > >>> > > > > > >> > > >>> > > > > > >> > >>> > > > > > >> >>> > > > > > > >>> > > > > > > >>> > > > > > >>> > > > > >>> > > >>> > >>> >> >> >
