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