kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rajini Sivaram <rajinisiva...@gmail.com>
Subject Re: [DISCUSS] KIP 226 - Dynamic Broker Configuration
Date Fri, 01 Dec 2017 16:32:11 GMT
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 <rajinisivaram@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 <rajinisivaram@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 <jason@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 <rajinisivaram@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 <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
>>> > > > > > >> > > > > >
>>> > > > > > >> > > > >
>>> > > > > > >> > > >
>>> > > > > > >> > >
>>> > > > > > >> >
>>> > > > > > >>
>>> > > > > > >
>>> > > > > > >
>>> > > > > >
>>> > > > >
>>> > >
>>> >
>>>
>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message