Hi Jason, Thank you for reviewing and voting.
Thanks, I had missed the rename. Have updated the KIP. The configs can be defined in the static server.properties or in ZooKeeper. If a ZK config cannot be decoded (or is not valid), we log an error and revert to the static config or default. When updating the secret used by the encode, we expect all password values to be specified in server.properties. And the decoding or sanity check of the password in ZK would fail with the new secret, so we would use the password values from server.properties. Once the broker starts up, the values can be reset in ZK using AdminClient and they will be encoded using the new secret. On Wed, Jan 3, 2018 at 5:34 PM, Jason Gustafson <ja...@confluent.io> wrote: > +1 Thanks for the KIP. One minor nit: I think we changed > ConfigSource.TOPIC_CONFIG to ConfigSource.DYNAMIC_TOPIC_CONFIG in the PR. > > As far as updating secrets, I wasn't sure I understand how that will work. > Do the password configs accept multiple values? > > On Wed, Jan 3, 2018 at 2:58 AM, Rajini Sivaram <rajinisiva...@gmail.com> > wrote: > > > Hi Jun, > > > > Thank you for reviewing and voting. > > > > 50. I have updated the KIP to describe how the secret may be changed. All > > dynamically configurable passwords and per-broker configs. So the secret > > can be different across brokers and updated using rolling restart. In > order > > to update the secret, each broker needs to be restarted with an updated > > server.properties which contains the new secret as well as the current > > values of all the password configs. Admin client can then be used to > update > > the passwords in ZooKeeper that are encrypted using the new secret. > > > > 51. leader.replication.throttled.replicas and > > follower.replication.throttled.replicas > > are dynamically configurable at the topic level. But there are no > defaults > > for these at the broker level since they refer to partitions of the > topic. > > The rates used for throttling were already configurable at the broker > > level. > > > > I made a couple of other changes to the KIP: > > > > 1. The config names used for encoding passwords are now prefixed with > > password.encoder. > > Also added key length as a config since this is constrained by the > > algorithm which is also configurable. > > 2. I moved the update of inter-broker security protocol and > > inter-broker sasl mechanism to the follow-on KIP under Future Work. As > part > > of the new KIP, we need to add protocol changes to validate that all > > brokers in the cluster support the new protocol/mechanism/version to > avoid > > accidental changes before all brokers are updated. > > > > > > On Tue, Jan 2, 2018 at 10:58 PM, Jun Rao <j...@confluent.io> wrote: > > > > > Hi, Rajini, > > > > > > Thank for the KIP. +1. Just a couple of minor comments below. > > > > > > > > > 50. config.secret.*: Could you document how the encryption/decryption > of > > > passwd work? In particular, how do we support changing config.secret? > > > > > > 51. At the topic level, we also have leader.replication.throttled. > > replicas > > > and follower.replication.throttled.replicas. Should they be > dynamically > > > configurable? > > > > > > Jun > > > > > > > > > > > > > > > > > > > > > On Tue, Dec 12, 2017 at 9:24 AM, Gwen Shapira <g...@confluent.io> > wrote: > > > > > > > +1 (binding). Thank you for leading this, Rajini. > > > > > > > > On Tue, Dec 12, 2017 at 8:35 AM Tom Bentley <t.j.bent...@gmail.com> > > > wrote: > > > > > > > > > +1 (nonbinding) > > > > > > > > > > On 12 December 2017 at 15:34, Ted Yu <yuzhih...@gmail.com> wrote: > > > > > > > > > > > +1 > > > > > > > > > > > > On Tue, Dec 12, 2017 at 5:44 AM, Rajini Sivaram < > > > > rajinisiva...@gmail.com > > > > > > > > > > > > wrote: > > > > > > > > > > > > > Since there are no more outstanding comments, I would like to > > start > > > > > vote > > > > > > > for KIP-226: > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > > > > > > 226+-+Dynamic+Broker+Configuration > > > > > > > > > > > > > > > > > > > > > The KIP enables dynamic update of commonly updated broker > > > > configuration > > > > > > > options to avoid expensive restarts. > > > > > > > > > > > > > > Thank you, > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > > > > > > > > > > > > >
