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 <cmcc...@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 <cmcc...@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 <cmcc...@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 <cmcc...@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
>> >> >> > > <viktorsomo...@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 <
>> >> >> > rajinisiva...@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 <yuzhih...@gmail.com>
>> >> >> > wrote:
>> >> >> > > > >
>> >> >> > > > > > +1
>> >> >> > > > > > -------- Original message --------From: zhenya Sun <
>> >> >> > toke...@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 <viktorsomo...@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 <
>> >> >> > mag...@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 <
>> >> >> > viktorsomo...@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 <
>> >> >> > > > > asasv...@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 <
>> >> >> > toke...@126.com>
>> >> >> > > > > > wrote:
>> >> >> > > > > > > > > > >
>> >> >> > > > > > > > > > >
>> >> >> > > > > > > > > > >
>> >> >> > > > > > > > > > > +1 （non-binding）
>> >> >> > > > > > > > > > >
>> >> >> > > > > > > > > > >
>> >> >> > > > > > > > > > > | |
>> >> >> > > > > > > > > > > zhenya Sun
>> >> >> > > > > > > > > > > 邮箱：toke...@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 <
>> >> >> > > > > > > > > viktorsomo...@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
>> >> >> > > > > > > > > > > >
>> >> >> > > > > > > > > >
>> >> >> > > > > > > > >
>> >> >> > > > > > > >
>> >> >> > > > > > >
>> >> >> > > > > >
>> >> >> > > > >
>> >> >> > > >
>> >> >> >
