kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Viktor Somogyi <viktorsomo...@gmail.com>
Subject Re: [VOTE] #2 KIP-248: Create New ConfigCommand That Uses The New AdminClient
Date Thu, 03 May 2018 12:11:55 GMT
@Magnus, yes that is correct. Thanks for your feedback. Updated it with
this (which might be subject to change based on the conversation with
Colin): "The changes done will be incremental in version 1, opposed to the
atomic behavior in version 0. For instance in version 0 sending an update
for producer_byte_rate for userA would result in removing all previous data
and setting userA's config with producer_byte_rate. Now in version 1
opposed to version 0 it will add an extra config and keeps other existing
configs."

@Colin,
yes, I have/had a hard time finding a place for this operation. I think ADD
and DELETE should be on config level to allow complex use cases (if someone
builds their own tool based on the AdminClient), so users can add and
delete multiple configs in one request.
But also at the same time, SET is as you're suggesting really seems like a
flag that tells the AdminClient/AdminManager how they should behave.
However since the AdminClient matches protocol version with the broker via
the API_VERSIONS request, I think it would be enough to modify the
AdminManager that it should behave differently in case of an increased
protocol versions, if there is this extra flag set through
AlterConfigOptions (AdminClient sets the flag on the protocol, which will
be reflected after parsing in AdminManager). Also if we target this change
to 2.0 (June?), then we might not need the extra flag but make the behavior
break. What do you think?

Keeping the --zookeeper option working is not infeasible of course - and as
per the community's feedback it may be the better option. Although one of
the goals is to put this new ConfigCommand to the tools module, which
doesn't have the dependency on the server code, it would be a bit harder.
Most likely I'd need to call into the Scala code with reflection, which
could be quite complicated.

Also rewrote the request semantics, hopefully it's more clear now.

Let me know what do you think about this and thank you for your feedback.

Cheers,
Viktor

On Thu, Apr 26, 2018 at 7:06 PM, Colin McCabe <cmccabe@apache.org> wrote:

> Hi Viktor,
>
> If I'm reading the KIP right, it looks like the new proposed verison of
> AlterConfigs sets an OperationType on a per-config basis:
>
>  > AlterConfigs Request (Version: 1) => [resources] validate_only
>  >   validate_only => BOOLEAN
>  >   resources => resource_type resource_name [configs]
>  >     resource_type => INT8
>  >     resource_name => STRING
>  >     configs => config_name config_value config_operation
>  >       config_name => STRING
>  >       config_value => STRING
>  >       config_operation => INT8 [NEW ADDITION]
>  >
>  > Request Semantics:
>  >
>  >      By default in the broker we parse an AlterConfigRequest version 0
>  > with Unknown operation and handle it with the currently existing
> behavior.
>  > Version 1 requests however must have the operation set to other than
>  > Unknown, otherwise an InvalidRequestException will be thrown.
>  >          Set operation also does Add if needed to be backward
> compatible
>  > with the existing ConfigCommand semantics.
>
> However, this seems like a configuration that should be global to the
> whole AlterConfigs request, right?  It doesn't make sense to have one
> configuration key use AlterOperation.Set and the other use
> AlterOperation.Add -- the Set one specifies that we should overwrite the
> whole node in ZK.
>
> Another consideration here is that we should do this in a compatible
> fashion in AdminClient.  Existing code that relies on the "set everything"
> behavior should not break.  The best way to do this is to add a boolean to
> ./clients/src/main/java/org/apache/kafka/clients/admin/AlterConfigsOptions.java
> , specifying whether we want to clear everything that hasn't been
> specified, or not.  This should default to true so that existing code can
> continue to work.... Unless we believe that the existing AlterConfigs
> behavior is so broken that it should be changed, even in a
> compatibility-breaking way.
>
> Similarly, for other tools, we managed to support both the zookeeper-based
> way and the new way in the same tool for a while.  This seems like
> something users would really want-- is it truly infeasible to do here?  The
> Java code could call into the Scala code as necessary when the zk flag was
> specified, right?
>
> best,
> Colin
>
>
> On Thu, Apr 26, 2018, at 01:47, Magnus Edenhill wrote:
> > Hi Viktor,
> >
> > after speaking to Rajini it seems like this KIP will allow clients to
> > perform incremental configuration updates with AlterConfigs, only
> providing
> > the settings
> > that it wants to change, as opposed to the current atomic behaviour where
> > all settings
> > need to be provided to avoid having them revert to their default values.
> >
> > If this is indeed the case, could you update the KIP to make this more
> > clear?
> > I.e., that using Version 1 of AlterConfigs has the incremental behaviour,
> > while
> > version 0 is atomic.
> >
> > Thanks,
> > Magnus
> >
> >
> >
> >
> > 2018-04-16 13:27 GMT+02:00 Viktor Somogyi <viktorsomogyi@gmail.com>:
> >
> > > Hi Rajini,
> > >
> > > The current ConfigCommand would still be possible to use, therefore
> those
> > > who wish to set up SCRAM or initial quotas would be able to continue
> doing
> > > it through kafka-run-class.sh.
> > > In an ideal world I'd keep it in the current ConfigCommand command so
> we
> > > wouldn't mix the zookeeper and admin client configs. Perhaps I could
> create
> > > a kafka-configs-zookeeper.sh shell script for shortening the
> > > kafka-run-class command.
> > > What do you and others think?
> > >
> > > Thanks,
> > > Viktor
> > >
> > >
> > >
> > > On Mon, Apr 16, 2018 at 10:15 AM, Rajini Sivaram <
> rajinisivaram@gmail.com>
> > > wrote:
> > >
> > > > Hi Viktor,
> > > >
> > > > The KIP proposes to remove the ability to configure using ZooKeeper.
> This
> > > > means we will no longer have the ability to start up a cluster with
> SCRAM
> > > > credentials since we first need to create SCRAM credentials before
> > > brokers
> > > > can start if the broker uses SCRAM for inter-broker communication
> and we
> > > > need SCRAM credentials for the AdminClient before we can create new
> ones.
> > > > For quotas as well, we will no longer be able to configure quotas
> until
> > > at
> > > > least one broker has been started. Perhaps, we ought to retain the
> > > ability
> > > > to configure using ZooKeeper for these initialization scenarios and
> > > support
> > > > only AdminClient for dynamic updates?
> > > >
> > > > What do others think?
> > > >
> > > > Regards,
> > > >
> > > > Rajini
> > > >
> > > > On Sun, Apr 15, 2018 at 10:28 AM, Ted Yu <yuzhihong@gmail.com>
> wrote:
> > > >
> > > > > +1
> > > > > -------- Original message --------From: zhenya Sun <
> token01@126.com>
> > > > > Date: 4/15/18  12:42 AM  (GMT-08:00) To: dev <dev@kafka.apache.org
> >
> > > Cc:
> > > > > dev <dev@kafka.apache.org> Subject: Re: [VOTE] #2 KIP-248:
Create
> New
> > > > > ConfigCommand That Uses The New AdminClient
> > > > > non-binding +1
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > from my iphone!
> > > > > On 04/15/2018 15:41, Attila Sasvári wrote:
> > > > > Thanks for updating the KIP.
> > > > >
> > > > > +1 (non-binding)
> > > > >
> > > > > Viktor Somogyi <viktorsomogyi@gmail.com> ezt írta (időpont:
2018.
> ápr.
> > > > 9.,
> > > > > H 16:49):
> > > > >
> > > > > > Hi Magnus,
> > > > > >
> > > > > > Thanks for the heads up, added the endianness to the KIP. Here
> is the
> > > > > > current text:
> > > > > >
> > > > > > "Double
> > > > > > A new type needs to be added to transfer quota values. Since
the
> > > > protocol
> > > > > > classes in Kafka already uses ByteBuffers it is logical to use
> their
> > > > > > functionality for serializing doubles. The serialization is
> > > basically a
> > > > > > representation of the specified floating-point value according
> to the
> > > > > IEEE
> > > > > > 754 floating-point "double format" bit layout. The ByteBuffer
> > > > serializer
> > > > > > writes eight bytes containing the given double value, in Big
> Endian
> > > > byte
> > > > > > order, into this buffer at the current position, and then
> increments
> > > > the
> > > > > > position by eight.
> > > > > >
> > > > > > The implementation will be defined in
> > > > > > org.apache.kafka.common.protocol.types with the other protocol
> types
> > > > > and it
> > > > > > will have no default value much like the other types available
> in the
> > > > > > protocol."
> > > > > >
> > > > > > Also, I haven't changed the protocol docs yet but will do so
in
> my PR
> > > > for
> > > > > > this feature.
> > > > > >
> > > > > > Let me know if you'd still add something.
> > > > > >
> > > > > > Regards,
> > > > > > Viktor
> > > > > >
> > > > > >
> > > > > > On Mon, Apr 9, 2018 at 3:32 PM, Magnus Edenhill <
> magnus@edenhill.se>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi Viktor,
> > > > > > >
> > > > > > > since serialization of floats isn't as straight forward
as
> > > integers,
> > > > > > please
> > > > > > > specify the exact serialization format of DOUBLE in the
> protocol
> > > docs
> > > > > > > (e.g., IEEE 754),
> > > > > > > including endianness (big-endian please).
> > > > > > >
> > > > > > > This will help the non-java client ecosystem.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Magnus
> > > > > > >
> > > > > > >
> > > > > > > 2018-04-09 15:16 GMT+02:00 Viktor Somogyi <
> viktorsomogyi@gmail.com
> > > >:
> > > > > > >
> > > > > > > > Hi Attila,
> > > > > > > >
> > > > > > > > 1. It uses ByteBuffers, which in turn will use
> > > > > Double.doubleToLongBits
> > > > > > to
> > > > > > > > convert the double value to a long and that long will
be
> written
> > > in
> > > > > the
> > > > > > > > buffer. I'v updated the KIP with this.
> > > > > > > > 2. Good idea, modified it.
> > > > > > > > 3. During the discussion I remember we didn't really
decide
> which
> > > > one
> > > > > > > would
> > > > > > > > be the better one but I agree that a wrapper class
that makes
> > > sure
> > > > > the
> > > > > > > list
> > > > > > > > that is used as a key is immutable is a good idea
and would
> ease
> > > > the
> > > > > > life
> > > > > > > > of people using the interface. Also more importantly
would
> make
> > > > sure
> > > > > > that
> > > > > > > > we always use the same hashCode. I have created wrapper
> classes
> > > for
> > > > > the
> > > > > > > map
> > > > > > > > value as well but that was reverted to keep things
> consistent.
> > > > > Although
> > > > > > > for
> > > > > > > > the key I think we wouldn't break consistency. I updated
the
> KIP.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Viktor
> > > > > > > >
> > > > > > > >
> > > > > > > > On Tue, Apr 3, 2018 at 1:27 PM, Attila Sasvári <
> > > > asasvari@apache.org>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Thanks for working on it Viktor.
> > > > > > > > >
> > > > > > > > > It looks good to me, but I have some questions:
> > > > > > > > > - I see a new type DOUBLE is used for quota_value
, and it
> is
> > > not
> > > > > > > listed
> > > > > > > > > among the primitive types on the Kafka protocol
guide. Can
> you
> > > > add
> > > > > > some
> > > > > > > > > more details?
> > > > > > > > > - I am not sure that using an environment (i.e.
> > > > > > > USE_OLD_COMMAND)variable
> > > > > > > > is
> > > > > > > > > the best way to control behaviour of kafka-config.sh
. In
> other
> > > > > > scripts
> > > > > > > > > (e.g. console-consumer) an argument is passed
(e.g.
> > > > > --new-consumer).
> > > > > > If
> > > > > > > > we
> > > > > > > > > still want to use it, then I would suggest something
like
> > > > > > > > > USE_OLD_KAFKA_CONFIG_COMMAND. What do you think?
> > > > > > > > > - I have seen maps like Map<List<ConfigResource>,
> > > > > > > Collection<QuotaType>>.
> > > > > > > > > If List<ConfigResource> is the key type,
you should make
> sure
> > > > that
> > > > > > this
> > > > > > > > > List is immutable. Have you considered to introduce
a new
> > > wrapper
> > > > > > > class?
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > - Attila
> > > > > > > > >
> > > > > > > > > On Thu, Mar 29, 2018 at 1:46 PM, zhenya Sun <
> token01@126.com>
> > > > > wrote:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > +1 (non-binding)
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > | |
> > > > > > > > > > zhenya Sun
> > > > > > > > > > 邮箱:token01@126.com
> > > > > > > > > > |
> > > > > > > > > >
> > > > > > > > > > 签名由 网易邮箱大师 定制
> > > > > > > > > >
> > > > > > > > > > On 03/29/2018 19:40, Sandor Murakozi wrote:
> > > > > > > > > > +1 (non-binding)
> > > > > > > > > >
> > > > > > > > > > Thanks for the KIP, Viktor
> > > > > > > > > >
> > > > > > > > > > On Wed, Mar 21, 2018 at 5:41 PM, Viktor
Somogyi <
> > > > > > > > viktorsomogyi@gmail.com
> > > > > > > > > >
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi Everyone,
> > > > > > > > > > >
> > > > > > > > > > > I've started a vote on KIP-248
> > > > > > > > > > > <https://cwiki.apache.org/conf
> luence/display/KAFKA/KIP-
> > > > > > > > > 248+-+Create+New+
> > > > > > > > > > > ConfigCommand+That+Uses+The+New+AdminClient#KIP-248-
> > > > > > > > > > > CreateNewConfigCommandThatUsesTheNewAdminClient-
> > > > > DescribeQuotas>
> > > > > > > > > > > a few weeks ago but at the time I got
a couple more
> > > comments
> > > > > and
> > > > > > it
> > > > > > > > was
> > > > > > > > > > > very close to 1.1 feature freeze, people
were occupied
> with
> > > > > that,
> > > > > > > so
> > > > > > > > I
> > > > > > > > > > > wanted to restart the vote on this.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > *Summary of the KIP*
> > > > > > > > > > > For those who don't have context I
thought I'd
> summarize it
> > > > in
> > > > > a
> > > > > > > few
> > > > > > > > > > > sentence.
> > > > > > > > > > > *Problem & Motivation: *The basic
problem that the KIP
> > > tries
> > > > to
> > > > > > > solve
> > > > > > > > > is
> > > > > > > > > > > that kafka-configs.sh (which in turn
uses the
> ConfigCommand
> > > > > > class)
> > > > > > > > uses
> > > > > > > > > a
> > > > > > > > > > > direct zookeeper connection. This is
not desirable as
> > > getting
> > > > > > > around
> > > > > > > > > the
> > > > > > > > > > > broker opens up security issues and
prevents the tool
> from
> > > > > being
> > > > > > > used
> > > > > > > > > in
> > > > > > > > > > > deployments where only the brokers
are exposed to
> clients.
> > > > > Also a
> > > > > > > > > somewhat
> > > > > > > > > > > smaller motivation is to rewrite the
tool in java as
> part
> > > of
> > > > > the
> > > > > > > > tools
> > > > > > > > > > > component so we can get rid of requiring
the core
> module on
> > > > the
> > > > > > > > > classpath
> > > > > > > > > > > for the kafka-configs tool.
> > > > > > > > > > > *Solution:*
> > > > > > > > > > > - I've designed new 2 protocols: DescribeQuotas
and
> > > > > AlterQuotas.
> > > > > > > > > > > - Also redesigned the output format
of the command line
> > > tool
> > > > so
> > > > > > it
> > > > > > > > > provides
> > > > > > > > > > > a nicer result.
> > > > > > > > > > > - kafka-configs.[sh/bat] will use a
new java based
> > > > > ConfigCommand
> > > > > > > that
> > > > > > > > > is
> > > > > > > > > > > placed in tools.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > I'd be happy to receive any votes or
feedback on this.
> > > > > > > > > > >
> > > > > > > > > > > Regards,
> > > > > > > > > > > Viktor
> > > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
>

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