kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ted Yu <yuzhih...@gmail.com>
Subject Re: [DISCUSS] KIP 226 - Dynamic Broker Configuration
Date Fri, 01 Dec 2017 18:28:19 GMT
Thanks for the explanation.

On Fri, Dec 1, 2017 at 10:13 AM, Rajini Sivaram <rajinisivaram@gmail.com>
wrote:

> Hi Ted,
>
> Thank you for the review. */config/brokers/id *contains the persistent
> configuration of broker *<id>. *That will be configured using
> kafka-configs.sh/AdminClient. */brokers/ids/id* contains the ephemeral
> metadata registered by broker *<id>.* For broker version, we don't want the
> data to outlive the broker. Hence adding it to */brokers/ids/id.*
>
> On Fri, Dec 1, 2017 at 5:38 PM, Ted Yu <yuzhihong@gmail.com> wrote:
>
> > Thanks for the update.
> >
> > bq. To enable this, the version of the broker will be added to the JSON
> > registered by each broker during startup at */brokers/ids/id*
> >
> > *It seems the path has typo. Should it be:*
> > */config/brokers/id*
> >
> > *Cheers*
> >
> > On Fri, Dec 1, 2017 at 8:32 AM, Rajini Sivaram <rajinisivaram@gmail.com>
> > wrote:
> >
> > > 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