Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
Message-Id: <1512500732.3800776.1195042448.6759936B@webmail.messagingengine.com>
From: Colin McCabe <cmccabe@apache.org>
To: dev@kafka.apache.org
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
Date: Tue, 05 Dec 2017 11:05:32 -0800
References: <CAOJcB3_dgGn=jiD-KjTkNKfub=1-WgeKVPEKtvDE-+tdr4GkPw@mail.gmail.com>
 <CALte62zBD9ivgpgMQtBA3V_9Y13wz__y6zR+q4_wiq_-7WqDYg@mail.gmail.com>
 <CAOJcB39A72ACR-4bG_yo6Evxz5CAzL1aZBYNfsdX5WW-aGdgTQ@mail.gmail.com>
 <CALte62wZQ3x9F38an4cBcDGzGwfstQrtwjDx-FaV9NDKttmDOw@mail.gmail.com>
 <CAOJcB38NOPVB4iG99g9eAALa1uWQ6iLH7a6n4wJC1wSBeC9rZw@mail.gmail.com>
 <CALte62yubHqjdSx+bu8AD1wuRk0QU88sAYm-4=Au=tseNoCkmw@mail.gmail.com>
 <CAOJcB3_=v=0003qYTX6gfrVw8xw536z8AZHPaYeOM30bdEyC9g@mail.gmail.com>
 <CAOJcB3_bc6ejE3s2HhQBjSZTXM-if3h+e2uxXr2QmTPMW_U7_Q@mail.gmail.com>
 <CAMd5Ysw_ab1yF_inn-JyN8HVpaq3+nr_7-PNeUUie1VjaWrNKw@mail.gmail.com>
 <CAOJcB3_Q5QA4FA8xANqzyONcBiU19cv7qdWcU3dOz3zhbsb7BQ@mail.gmail.com>
 <1511898471.3563368.1187212376.37C67248@webmail.messagingengine.com>
 <CAOJcB3-ep5OAOPiQT5EzTonJnB6YDod7XhC+W7tVuKDjubHccw@mail.gmail.com>
 <1512252923.246291.1191922032.307CDBFA@webmail.messagingengine.com>
 <CAOJcB3-HiT=H8PEFK-33yswSdoOUF1zcprQ+cNR6HWsgCNXOXg@mail.gmail.com>
Subject: Re: [DISCUSS] KIP 226 - Dynamic Broker Configuration
In-Reply-To: <CAOJcB3-HiT=H8PEFK-33yswSdoOUF1zcprQ+cNR6HWsgCNXOXg@mail.gmail.com>
archived-at: Tue, 05 Dec 2017 19:05:42 -0000

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 <cmccabe@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 <cmccabe@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.bentley@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 <
> > rajinisivaram@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 <
> > > > > > rajinisivaram@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 <yuzhihong@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 <
> > > > > > > rajinisivaram@gmail.com
> > > > > > > >> >
> > > > > > > >> wrote:
> > > > > > > >>
> > > > > > > >> > Ted,
> > > > > > > >> >
> > > > > > > >> > Have added an example for --delete-config.
> > > > > > > >> >
> > > > > > > >> > On Mon, Nov 20, 2017 at 7:42 PM, Ted Yu <
> > yuzhihong@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 <
> > > > > > > >> > rajinisivaram@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 <
> > > > yuzhihong@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 <
> > > > > > > >> > > rajinisivaram@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
> > > > > > > >> > > > > >
> > > > > > > >> > > > >
> > > > > > > >> > > >
> > > > > > > >> > >
> > > > > > > >> >
> > > > > > > >>
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> >