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 18:13:36 GMT
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