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