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
> >> >> > > > > > > > > > > >
> >> >> > > > > > > > > >
> >> >> > > > > > > > >
> >> >> > > > > > > >
> >> >> > > > > > >
> >> >> > > > > >
> >> >> > > > >
> >> >> > > >
> >> >> >
