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 Wed, 16 May 2018 11:57:36 GMT
Hi Colin,

> Doing get-merge-set is buggy, though.  If someone else does get-merge-set at the same
time as you, you might overwrite that person's changes, or vice versa.  So I really don't
think we should try to do this.  Also, having both an incremental and a full API is useful,
and it's just a single boolean at the protocol and API level.

Overwriting somebody's change is currently possible with the
ConfigCommand, as it will do this get-merge-set behavior on the client
side, in the command. From this perspective I think it's not much
different to do this with the admin client. Also I think admins don't
modify the quotas/configs of a client/user/topic/broker often (and
multiple admins would do it even more rarely), so I don't think it is
a big issue. What I think would be useful here but may be out of scope
is to version the changes similarly to leader epochs. So when an admin
updates the configs, it will increment a version number and won't let
other admins to push changes in with lower than that. Instead it would
return an error.

I would be also interested what others think about this?

Cheers,
Viktor


On Sat, May 12, 2018 at 2:29 AM, Colin McCabe <cmccabe@apache.org> wrote:
> On Wed, May 9, 2018, at 05:41, Viktor Somogyi wrote:
>> Hi Colin,
>>
>> > We are going to need to create a new version of AlterConfigsRequest to add the
"incremental" boolean.  So while we're doing that, maybe we can change the type to NULLABLE_STRING.
>>
>> I was just talking to a colleague yesterday and we came to the
>> conclusion that we should keep the boolean flag only on the client
>> side (as you may have suggested earlier?) and not make part of the
>> protocol as it might lead to a very complicated API on the long term.
>> Also we would keep the server side API simpler. Instead of the
>> protocol change we could just simply have the boolean flag in
>> AlterConfigOptions and the AdminClient should do the get-merge-set
>> logic which corresponds to the behavior of the current ConfigCommand.
>> That way we won't need to change the protocol for now but still have
>> both functionality. What do you think?
>
>  Hi Viktor,
>
> Doing get-merge-set is buggy, though.  If someone else does get-merge-set at the same
time as you, you might overwrite that person's changes, or vice versa.  So I really don't
think we should try to do this.  Also, having both an incremental and a full API is useful,
and it's just a single boolean at the protocol and API level.
>
>>
>> > Hmm.  Not sure I follow.  KIP-133 doesn't use the empty string or "<default>"
to indicate defaults, does it?
>>
>> No it doesn't. It was just my early idea to indicate "delete" on the
>> protocol level. (We are using <default> for denoting the default
>> client id or user in zookeeper.) Rajini was referring that we
>> shouldn't expose this to the protocol level but instead denote delete
>> with an empty string.
>>
>> > This comes from DescribeConfigsResponse.
>> > Unless I'm missing something, I think your suggestion to not expose "<default>"
is already implemented?
>>
>> In some way, yes. Although this one is used in describe and not in
>> alter. For alter I don't think we'd need my early "<default>" idea.
>
> OK.  Thanks for the explanation.  Using an empty string to indicate delete, as Rajini
suggested, seems pretty reasonable to me.  null would work as well.
>
>>
>> >> And we use STRING rather than NULLABLE_STRING in describe configs etc. So
we
>> >> should probably do the same for quotas."
>> >
>> > I think nearly all responses treat ERROR_MESSAGE as a nullable string.  CommonFields#ERROR_MESSAGE,
which is used by most of them, is a nullable string.  It's DescribeConfigsResponse that is
the black sheep here.
>> >
>> >  >     public static final Field.NullableStr ERROR_MESSAGE = new Field.NullableStr("error_message",
"Response error message");
>>
>> Looking at DescribeConfigsResponse (and AlterConfigsResponse) they use
>> nullable_string in the code. KIP-133 states otherwise though. So in
>> this case it's not a problem luckily.
>
> Thanks for finding this inconsistency.  I'll change the KIP to reflect what was actually
implemented (nullable string for error).
>
> cheers,
> Colin
>
>>
>> > What about writing a small script that just handles setting up SCRAM credentials?
 It would probably be easier to maintain than the old config command.  Otherwise we have to
explain when each tool should be used, which will be confusing to users.
>>
>> I'd like that, yes :).
>>
>> Cheers,
>> Viktor
>>
>> On Mon, May 7, 2018 at 6:52 PM, Colin McCabe <cmccabe@apache.org> wrote:
>> > On Fri, May 4, 2018, at 05:49, Viktor Somogyi wrote:
>> >> Hi Colin,
>> >>
>> >> > Rather than breaking compatibility, we should simply add a new "incremental"
boolean to AlterConfigsOptions.  Callers can set this boolean to true when they want the update
to be incremental.  It should default to false so that old code continues to work.
>> >>
>> >> Agreed, let's do it this way.
>> >>
>> >> > Hmm.  I don't think AlterOperation is necessary.  If the user wants
to delete a configuration key named "foo", they can create a ConfigEntry with name = "foo",
value = null.
>> >>
>> >> AlterConfig's config type currently is string, so the only possibility
>> >> is to use an empty string as changing the type to nullable_string
>> >> could be breaking if the client code doesn't expect -1 as the string
>> >> size. In the discussion thread earlier we had a conversation about
>> >> this with Rajini, let me paste it here (so it gives some context). At
>> >> that point I had the text "<default>" for this functionality:
>> >
>> > Hi Viktor,
>> >
>> > We are going to need to create a new version of AlterConfigsRequest to add the
"incremental" boolean.  So while we're doing that, maybe we can change the type to NULLABLE_STRING.
>> >
>> >> "4. We use "<default>" internally to store default quotas and other
>> >> defaults. But I don't think we should externalise that string. We use empty
>> >> string elsewhere for indicating default, we can do the same here.
>> >
>> > Hmm.  Not sure I follow.  KIP-133 doesn't use the empty string or "<default>"
to indicate defaults, does it?
>> >
>> > There is a ConfigEntry class:
>> >
>> >  > @InterfaceStability.Evolving
>> >  > public class ConfigEntry {
>> >  >
>> >  >     private final String name;
>> >  >     private final String value;
>> >  >     private final ConfigSource source;
>> >  >     private final boolean isSensitive;
>> >  >     private final boolean isReadOnly;
>> >  >     private final List<ConfigSynonym> synonyms;
>> >
>> > and the ConfigSource enum indicates where the config came from:
>> >
>> >  >     /**
>> >  >      * Source of configuration entries.
>> >  >      */
>> >  >     public enum ConfigSource {
>> >  >         DYNAMIC_TOPIC_CONFIG,           // dynamic topic config that is
configured for a specific topic
>> >  >         DYNAMIC_BROKER_CONFIG,          // dynamic broker config that
is configured for a specific broker
>> >  >         DYNAMIC_DEFAULT_BROKER_CONFIG,  // dynamic broker config that
is configured as default for all brokers in the cluster
>> >  >         STATIC_BROKER_CONFIG,           // static broker config provided
as broker properties at start up (e.g. server.properties file)
>> >  >         DEFAULT_CONFIG,                 // built-in default configuration
for configs that have a default value
>> >  >         UNKNOWN                         // source unknown e.g. in the
ConfigEntry used for alter requests where source is not set
>> >  >     }
>> >
>> > This comes from DescribeConfigsResponse.
>> > Unless I'm missing something, I think your suggestion to not expose "<default>"
is already implemented?
>> >
>> >> And we use STRING rather than NULLABLE_STRING in describe configs etc. So
we
>> >> should probably do the same for quotas."
>> >
>> > I think nearly all responses treat ERROR_MESSAGE as a nullable string.  CommonFields#ERROR_MESSAGE,
which is used by most of them, is a nullable string.  It's DescribeConfigsResponse that is
the black sheep here.
>> >
>> >  >     public static final Field.NullableStr ERROR_MESSAGE = new Field.NullableStr("error_message",
"Response error message");
>> >
>> >>
>> >> > Yeah, this might be an excessive maintenance burden.  Maybe we should
get rid of the old zookeeper-based code, and just move towards having only a KIP-248-based
tool.  It's a breaking change, but it's clear to users that it's occurring, and what the fix
is (specifying --bootstrap-server instead of --zookeeper).
>> >>
>> >> Earlier Rajini raised a concern that direct zookeeper interaction is
>> >> required to add the SCRAM credentials which will be used for
>> >> validation if inter-broker communication uses this auth method. This
>> >> is currently done by the ConfigCommand. Therefore we can't completely
>> >> get rid of it yet either.
>> >>
>> >> In my opinion though on a longer term (and this is now a bit
>> >> off-topic) Kafka shouldn't use Zookeeper as a credentials store, just
>> >> provide an interface, so 3rd party authentication stores could be
>> >> implemented. Then similarly to the authorizer we could have Zookeeper
>> >> as a default though and a client that manages SCRAM credentials in ZK.
>> >> From this perspective I'd leave the the command there but put a
>> >> warning that the tool is deprecated and should only be used for
>> >> setting up SCRAM credentials.
>> >> What do you think?
>> >
>> > What about writing a small script that just handles setting up SCRAM credentials?
 It would probably be easier to maintain than the old config command.  Otherwise we have to
explain when each tool should be used, which will be confusing to users.
>> >
>> > best,
>> > Colin
>> >
>> >>
>> >> Cheers,
>> >> Viktor
>> >>
>> >>
>> >> On Thu, May 3, 2018 at 7:47 PM, Colin McCabe <cmccabe@apache.org>
wrote:
>> >> > On Thu, May 3, 2018, at 05:11, Viktor Somogyi wrote:
>> >> >> @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."
>> >> >
>> >> > Hi Viktor,
>> >> >
>> >> > AdminClient#alterConfigs is a public API which users have already written
code against.  If we silently change what it does to be incremental addition rather than complete
replacement of the existing configuration, we will break all of that existing code.  If we
do that, there is not even any way that users can write code to support both broker versions.
 AdminClient does not expose any API that users can use to check broker version.  I think
that would be really bad for users.
>> >> >
>> >> > Rather than breaking compatibility, we should simply add a new "incremental"
boolean to AlterConfigsOptions.  Callers can set this boolean to true when they want the update
to be incremental.  It should default to false so that old code continues to work.
>> >> >
>> >> >>
>> >> >> @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.
>> >> >
>> >> > Hmm.  I don't think AlterOperation is necessary.  If the user wants
to delete a configuration key named "foo", they can create a ConfigEntry with name = "foo",
value = null.
>> >> >
>> >> >> 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?
>> >> >
>> >> > Right.  I think a flag in AlterConfigsRequest makes sense.  AdminClient
can set it based on a boolean field in AlterConfigsOptions.
>> >> >
>> >> >>
>> >> >> 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.
>> >> >
>> >> > Yeah, this might be an excessive maintenance burden.  Maybe we should
get rid of the old zookeeper-based code, and just move towards having only a KIP-248-based
tool.  It's a breaking change, but it's clear to users that it's occurring, and what the fix
is (specifying --bootstrap-server instead of --zookeeper).
>> >> >
>> >> > best,
>> >> > Colin
>> >> >
>> >> >>
>> >> >> 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
View raw message