You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Aneel Nazareth <an...@confluent.io> on 2020/02/26 18:36:17 UTC
[DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Hi,
I'd like to discuss adding a new argument to kafka-configs.sh
(ConfigCommand.scala).
Recently I've been working on some things that require complex
configurations. I've chosen to represent them as JSON strings in my
server.properties. This works well, and I'm able to update the
configurations by editing server.properties and restarting the broker. I've
added the ability to dynamically configure them, and that works well using
the AdminClient. However, when I try to update these configurations using
kafka-configs.sh, I run into a problem. My configurations contain commas,
and kafka-configs.sh tries to break them up into key/value pairs at the
comma boundary.
I'd like to enable setting these configurations from the command line, so
I'm proposing that we add a new option to kafka-configs.sh that takes a
properties file.
I've created a KIP for this idea:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
I'd appreciate your feedback on the proposal.
Thanks,
Aneel
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Aneel Nazareth <an...@confluent.io>.
Hi Colin,
Thanks for the suggestion. Using a dash does seem reasonable. I can
make that change.
On Mon, Mar 9, 2020 at 12:35 PM Colin McCabe <cm...@apache.org> wrote:
>
> Hi Aneel,
>
> Thanks for the KIP. I like the idea.
>
> You mention that "input from STDIN can be used instead of a file on disk." The example given in the KIP seems to suggest that the command defaults to reading from STDIN if no argument is given to --add-config-file.
>
> I would argue against this particular command-line pattern. From the user's point of view, if they mess up and forget to supply an argument, or for some reason the parser doesn't treat something like an argument, the program will appear to hang in a confusing way.
>
> Instead, it would be better to follow the traditional UNIX pattern where a dash indicates that STDIN should be read. So "--add-config-file -" would indicate that the program should read form STDIN. This would be difficult to trigger accidentally, and more in line with the traditional conventions.
>
> On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > I wonder if we should also add a `--delete-config-file` as a counterpart of
> > `--add-config-file`. It would be a bit weird to use a properties file in
> > this case as the values are not necessary but it may be handy to have the
> > possibility to remove the configurations which have been set. Have you
> > considered this?
>
> Hi David,
>
> That's an interesting idea. However, I think it might be confusing to users to supply a file, and then have the values supplied in that file be ignored. Is there really a case where we need to do this (as opposed to creating a file with blank values, or just passing the keys to --delete-config?
>
> best,
> Colin
>
> >
> > David
> >
> > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <an...@confluent.io> wrote:
> >
> > > I've created a PR for a potential implementation of this:
> > > https://github.com/apache/kafka/pull/8184 if we decide to go ahead with
> > > this KIP.
> > >
> > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <an...@confluent.io>
> > > wrote:
> > >
> > > > Hi,
> > > >
> > > > I'd like to discuss adding a new argument to kafka-configs.sh
> > > > (ConfigCommand.scala).
> > > >
> > > > Recently I've been working on some things that require complex
> > > > configurations. I've chosen to represent them as JSON strings in my
> > > > server.properties. This works well, and I'm able to update the
> > > > configurations by editing server.properties and restarting the broker.
> > > I've
> > > > added the ability to dynamically configure them, and that works well
> > > using
> > > > the AdminClient. However, when I try to update these configurations using
> > > > kafka-configs.sh, I run into a problem. My configurations contain commas,
> > > > and kafka-configs.sh tries to break them up into key/value pairs at the
> > > > comma boundary.
> > > >
> > > > I'd like to enable setting these configurations from the command line, so
> > > > I'm proposing that we add a new option to kafka-configs.sh that takes a
> > > > properties file.
> > > >
> > > > I've created a KIP for this idea:
> > > >
> > > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
> > > >
> > > > I'd appreciate your feedback on the proposal.
> > > >
> > > > Thanks,
> > > > Aneel
> > > >
> > >
> >
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Colin McCabe <cm...@apache.org>.
I agree with this. I think we'd be better off without the --delete-config-file option. It just seems confusing.
best,
Colin
On Wed, Mar 25, 2020, at 03:48, Rajini Sivaram wrote:
> Hi Aneel,
>
> Thanks for the KIP. As configurations get more complex, the ability to
> provide compound configs in a file is really useful. I am not convinced
> about the `--delete-config-file` option though. I am not familiar with the
> Kubernetes case, but I guess if you create an entity with a file, it is
> reasonable to delete the entity with the same config file, even though some
> configs of the entity may have changed. But like their non-file
> counterparts, `--add-config-file` and `--delete-config-file` aren't
> creating or deleting anything, they are both updating configs of an entity.
> To be precise, they are updating config overrides, one adds an element to a
> hierarachy and the other removes an element from a hierarchy. The current
> proposal allows you to update configs of entityA using fileA and then
> delete configs of entityB using fileA, with totally unintended consequences
> since config values are not validated. Since configs are like maps, it is
> confusing if delete with value doesn't have the semantics of remove(key,
> value). And the option to specify both `--delete-config` and `
> --delete-config-file` just makes this option inconsistent. Do we really
> need a `--delete-config-file` option?
>
> Regards,
>
> Rajini
>
> On Wed, Mar 25, 2020 at 8:25 AM Kamal Chandraprakash <
> kamal.chandraprakash@gmail.com> wrote:
>
> > STDIN wasn't standard practice in other scripts like
> > kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> > in which the props file is accepted via consumer.config / producer.config /
> > command-config parameter.
> >
> > Shouldn't we have to maintain the uniformity across scripts?
> >
> > On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io> wrote:
> >
> > > Hi Aneel,
> > >
> > > Thanks for the updated KIP. I have made a second pass over it and the
> > > KIP looks good to me.
> > >
> > > Best,
> > > David
> > >
> > > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io>
> > wrote:
> > >
> > > > After reading a bit more about it in the Kubernetes case, I think it's
> > > > reasonable to do this and be explicit that we're ignoring the value,
> > > > just deleting all keys that appear in the file.
> > > >
> > > > I've updated the KIP wiki page to reflect that:
> > > >
> > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > >
> > > > And updated my sample PR:
> > > > https://github.com/apache/kafka/pull/8184
> > > >
> > > > If there are no further comments, I'll request a vote in a few days.
> > > >
> > > > Thanks for the feedback!
> > > >
> > > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > > wrote:
> > > > >
> > > > > Hi David,
> > > > >
> > > > > Is the expected behavior that the keys are deleted without checking
> > the
> > > > values?
> > > > >
> > > > > Let's say I had this file new.properties:
> > > > > a=1
> > > > > b=2
> > > > >
> > > > > And ran:
> > > > >
> > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > --entity-type brokers --entity-default \
> > > > > --alter --add-config-file new.properties
> > > > >
> > > > > It seems clear what should happen if I run this immediately:
> > > > >
> > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > --entity-type brokers --entity-default \
> > > > > --alter --delete-config-file new.properties
> > > > >
> > > > > (Namely that both a and b would now have no values in the config)
> > > > >
> > > > > But what if this were run in-between:
> > > > >
> > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > --entity-type brokers --entity-default \
> > > > > --alter --add-config a=3
> > > > >
> > > > > Would it be surprising if the key/value pair a=3 was deleted, even
> > > > > though the config that is in the file is a=1? Or would that be
> > > > > expected?
> > > > >
> > > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > > wrote:
> > > > > >
> > > > > > Hi Colin,
> > > > > >
> > > > > > Yes, you're right. This is weird but convenient because you don't
> > > have
> > > > to
> > > > > > duplicate
> > > > > > the "keys". I was thinking about the kubernetes API which allows to
> > > > create
> > > > > > a Pod
> > > > > > based on a file and allows to delete it as well with the same
> > file. I
> > > > have
> > > > > > always found
> > > > > > this convenient, especially when doing local tests.
> > > > > >
> > > > > > Best,
> > > > > > David
> > > > > >
> > > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <cm...@apache.org>
> > > > wrote:
> > > > > >
> > > > > > > Hi Aneel,
> > > > > > >
> > > > > > > Thanks for the KIP. I like the idea.
> > > > > > >
> > > > > > > You mention that "input from STDIN can be used instead of a file
> > on
> > > > > > > disk." The example given in the KIP seems to suggest that the
> > > > command
> > > > > > > defaults to reading from STDIN if no argument is given to
> > > > --add-config-file.
> > > > > > >
> > > > > > > I would argue against this particular command-line pattern. From
> > > the
> > > > > > > user's point of view, if they mess up and forget to supply an
> > > > argument, or
> > > > > > > for some reason the parser doesn't treat something like an
> > > argument,
> > > > the
> > > > > > > program will appear to hang in a confusing way.
> > > > > > >
> > > > > > > Instead, it would be better to follow the traditional UNIX
> > pattern
> > > > where a
> > > > > > > dash indicates that STDIN should be read. So "--add-config-file
> > -"
> > > > would
> > > > > > > indicate that the program should read form STDIN. This would be
> > > > difficult
> > > > > > > to trigger accidentally, and more in line with the traditional
> > > > conventions.
> > > > > > >
> > > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > > I wonder if we should also add a `--delete-config-file` as a
> > > > counterpart
> > > > > > > of
> > > > > > > > `--add-config-file`. It would be a bit weird to use a
> > properties
> > > > file in
> > > > > > > > this case as the values are not necessary but it may be handy
> > to
> > > > have the
> > > > > > > > possibility to remove the configurations which have been set.
> > > Have
> > > > you
> > > > > > > > considered this?
> > > > > > >
> > > > > > > Hi David,
> > > > > > >
> > > > > > > That's an interesting idea. However, I think it might be
> > confusing
> > > > to
> > > > > > > users to supply a file, and then have the values supplied in that
> > > > file be
> > > > > > > ignored. Is there really a case where we need to do this (as
> > > > opposed to
> > > > > > > creating a file with blank values, or just passing the keys to
> > > > > > > --delete-config?
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > > > >
> > > > > > > > David
> > > > > > > >
> > > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > > aneel@confluent.io>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide to go
> > > > ahead
> > > > > > > with
> > > > > > > > > this KIP.
> > > > > > > > >
> > > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > > aneel@confluent.io>
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > I'd like to discuss adding a new argument to
> > kafka-configs.sh
> > > > > > > > > > (ConfigCommand.scala).
> > > > > > > > > >
> > > > > > > > > > Recently I've been working on some things that require
> > > complex
> > > > > > > > > > configurations. I've chosen to represent them as JSON
> > strings
> > > > in my
> > > > > > > > > > server.properties. This works well, and I'm able to update
> > > the
> > > > > > > > > > configurations by editing server.properties and restarting
> > > the
> > > > > > > broker.
> > > > > > > > > I've
> > > > > > > > > > added the ability to dynamically configure them, and that
> > > > works well
> > > > > > > > > using
> > > > > > > > > > the AdminClient. However, when I try to update these
> > > > configurations
> > > > > > > using
> > > > > > > > > > kafka-configs.sh, I run into a problem. My configurations
> > > > contain
> > > > > > > commas,
> > > > > > > > > > and kafka-configs.sh tries to break them up into key/value
> > > > pairs at
> > > > > > > the
> > > > > > > > > > comma boundary.
> > > > > > > > > >
> > > > > > > > > > I'd like to enable setting these configurations from the
> > > > command
> > > > > > > line, so
> > > > > > > > > > I'm proposing that we add a new option to kafka-configs.sh
> > > that
> > > > > > > takes a
> > > > > > > > > > properties file.
> > > > > > > > > >
> > > > > > > > > > I've created a KIP for this idea:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > > And a JIRA:
> > https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > > >
> > > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Aneel
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Aneel Nazareth <an...@confluent.io>.
Hi Rajini,
Thanks for taking a look at this. It seems like the consensus is to
remove the --delete-config-file option, so I'll do so.
On Wed, Mar 25, 2020 at 5:48 AM Rajini Sivaram <ra...@gmail.com> wrote:
>
> Hi Aneel,
>
> Thanks for the KIP. As configurations get more complex, the ability to
> provide compound configs in a file is really useful. I am not convinced
> about the `--delete-config-file` option though. I am not familiar with the
> Kubernetes case, but I guess if you create an entity with a file, it is
> reasonable to delete the entity with the same config file, even though some
> configs of the entity may have changed. But like their non-file
> counterparts, `--add-config-file` and `--delete-config-file` aren't
> creating or deleting anything, they are both updating configs of an entity.
> To be precise, they are updating config overrides, one adds an element to a
> hierarachy and the other removes an element from a hierarchy. The current
> proposal allows you to update configs of entityA using fileA and then
> delete configs of entityB using fileA, with totally unintended consequences
> since config values are not validated. Since configs are like maps, it is
> confusing if delete with value doesn't have the semantics of remove(key,
> value). And the option to specify both `--delete-config` and `
> --delete-config-file` just makes this option inconsistent. Do we really
> need a `--delete-config-file` option?
>
> Regards,
>
> Rajini
>
> On Wed, Mar 25, 2020 at 8:25 AM Kamal Chandraprakash <
> kamal.chandraprakash@gmail.com> wrote:
>
> > STDIN wasn't standard practice in other scripts like
> > kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> > in which the props file is accepted via consumer.config / producer.config /
> > command-config parameter.
> >
> > Shouldn't we have to maintain the uniformity across scripts?
> >
> > On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io> wrote:
> >
> > > Hi Aneel,
> > >
> > > Thanks for the updated KIP. I have made a second pass over it and the
> > > KIP looks good to me.
> > >
> > > Best,
> > > David
> > >
> > > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io>
> > wrote:
> > >
> > > > After reading a bit more about it in the Kubernetes case, I think it's
> > > > reasonable to do this and be explicit that we're ignoring the value,
> > > > just deleting all keys that appear in the file.
> > > >
> > > > I've updated the KIP wiki page to reflect that:
> > > >
> > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > >
> > > > And updated my sample PR:
> > > > https://github.com/apache/kafka/pull/8184
> > > >
> > > > If there are no further comments, I'll request a vote in a few days.
> > > >
> > > > Thanks for the feedback!
> > > >
> > > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > > wrote:
> > > > >
> > > > > Hi David,
> > > > >
> > > > > Is the expected behavior that the keys are deleted without checking
> > the
> > > > values?
> > > > >
> > > > > Let's say I had this file new.properties:
> > > > > a=1
> > > > > b=2
> > > > >
> > > > > And ran:
> > > > >
> > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > --entity-type brokers --entity-default \
> > > > > --alter --add-config-file new.properties
> > > > >
> > > > > It seems clear what should happen if I run this immediately:
> > > > >
> > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > --entity-type brokers --entity-default \
> > > > > --alter --delete-config-file new.properties
> > > > >
> > > > > (Namely that both a and b would now have no values in the config)
> > > > >
> > > > > But what if this were run in-between:
> > > > >
> > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > --entity-type brokers --entity-default \
> > > > > --alter --add-config a=3
> > > > >
> > > > > Would it be surprising if the key/value pair a=3 was deleted, even
> > > > > though the config that is in the file is a=1? Or would that be
> > > > > expected?
> > > > >
> > > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > > wrote:
> > > > > >
> > > > > > Hi Colin,
> > > > > >
> > > > > > Yes, you're right. This is weird but convenient because you don't
> > > have
> > > > to
> > > > > > duplicate
> > > > > > the "keys". I was thinking about the kubernetes API which allows to
> > > > create
> > > > > > a Pod
> > > > > > based on a file and allows to delete it as well with the same
> > file. I
> > > > have
> > > > > > always found
> > > > > > this convenient, especially when doing local tests.
> > > > > >
> > > > > > Best,
> > > > > > David
> > > > > >
> > > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <cm...@apache.org>
> > > > wrote:
> > > > > >
> > > > > > > Hi Aneel,
> > > > > > >
> > > > > > > Thanks for the KIP. I like the idea.
> > > > > > >
> > > > > > > You mention that "input from STDIN can be used instead of a file
> > on
> > > > > > > disk." The example given in the KIP seems to suggest that the
> > > > command
> > > > > > > defaults to reading from STDIN if no argument is given to
> > > > --add-config-file.
> > > > > > >
> > > > > > > I would argue against this particular command-line pattern. From
> > > the
> > > > > > > user's point of view, if they mess up and forget to supply an
> > > > argument, or
> > > > > > > for some reason the parser doesn't treat something like an
> > > argument,
> > > > the
> > > > > > > program will appear to hang in a confusing way.
> > > > > > >
> > > > > > > Instead, it would be better to follow the traditional UNIX
> > pattern
> > > > where a
> > > > > > > dash indicates that STDIN should be read. So "--add-config-file
> > -"
> > > > would
> > > > > > > indicate that the program should read form STDIN. This would be
> > > > difficult
> > > > > > > to trigger accidentally, and more in line with the traditional
> > > > conventions.
> > > > > > >
> > > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > > I wonder if we should also add a `--delete-config-file` as a
> > > > counterpart
> > > > > > > of
> > > > > > > > `--add-config-file`. It would be a bit weird to use a
> > properties
> > > > file in
> > > > > > > > this case as the values are not necessary but it may be handy
> > to
> > > > have the
> > > > > > > > possibility to remove the configurations which have been set.
> > > Have
> > > > you
> > > > > > > > considered this?
> > > > > > >
> > > > > > > Hi David,
> > > > > > >
> > > > > > > That's an interesting idea. However, I think it might be
> > confusing
> > > > to
> > > > > > > users to supply a file, and then have the values supplied in that
> > > > file be
> > > > > > > ignored. Is there really a case where we need to do this (as
> > > > opposed to
> > > > > > > creating a file with blank values, or just passing the keys to
> > > > > > > --delete-config?
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > > > >
> > > > > > > > David
> > > > > > > >
> > > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > > aneel@confluent.io>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide to go
> > > > ahead
> > > > > > > with
> > > > > > > > > this KIP.
> > > > > > > > >
> > > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > > aneel@confluent.io>
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > I'd like to discuss adding a new argument to
> > kafka-configs.sh
> > > > > > > > > > (ConfigCommand.scala).
> > > > > > > > > >
> > > > > > > > > > Recently I've been working on some things that require
> > > complex
> > > > > > > > > > configurations. I've chosen to represent them as JSON
> > strings
> > > > in my
> > > > > > > > > > server.properties. This works well, and I'm able to update
> > > the
> > > > > > > > > > configurations by editing server.properties and restarting
> > > the
> > > > > > > broker.
> > > > > > > > > I've
> > > > > > > > > > added the ability to dynamically configure them, and that
> > > > works well
> > > > > > > > > using
> > > > > > > > > > the AdminClient. However, when I try to update these
> > > > configurations
> > > > > > > using
> > > > > > > > > > kafka-configs.sh, I run into a problem. My configurations
> > > > contain
> > > > > > > commas,
> > > > > > > > > > and kafka-configs.sh tries to break them up into key/value
> > > > pairs at
> > > > > > > the
> > > > > > > > > > comma boundary.
> > > > > > > > > >
> > > > > > > > > > I'd like to enable setting these configurations from the
> > > > command
> > > > > > > line, so
> > > > > > > > > > I'm proposing that we add a new option to kafka-configs.sh
> > > that
> > > > > > > takes a
> > > > > > > > > > properties file.
> > > > > > > > > >
> > > > > > > > > > I've created a KIP for this idea:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > > And a JIRA:
> > https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > > >
> > > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Aneel
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > >
> > >
> >
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Rajini Sivaram <ra...@gmail.com>.
Hi Aneel,
Thanks for the KIP. As configurations get more complex, the ability to
provide compound configs in a file is really useful. I am not convinced
about the `--delete-config-file` option though. I am not familiar with the
Kubernetes case, but I guess if you create an entity with a file, it is
reasonable to delete the entity with the same config file, even though some
configs of the entity may have changed. But like their non-file
counterparts, `--add-config-file` and `--delete-config-file` aren't
creating or deleting anything, they are both updating configs of an entity.
To be precise, they are updating config overrides, one adds an element to a
hierarachy and the other removes an element from a hierarchy. The current
proposal allows you to update configs of entityA using fileA and then
delete configs of entityB using fileA, with totally unintended consequences
since config values are not validated. Since configs are like maps, it is
confusing if delete with value doesn't have the semantics of remove(key,
value). And the option to specify both `--delete-config` and `
--delete-config-file` just makes this option inconsistent. Do we really
need a `--delete-config-file` option?
Regards,
Rajini
On Wed, Mar 25, 2020 at 8:25 AM Kamal Chandraprakash <
kamal.chandraprakash@gmail.com> wrote:
> STDIN wasn't standard practice in other scripts like
> kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> in which the props file is accepted via consumer.config / producer.config /
> command-config parameter.
>
> Shouldn't we have to maintain the uniformity across scripts?
>
> On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io> wrote:
>
> > Hi Aneel,
> >
> > Thanks for the updated KIP. I have made a second pass over it and the
> > KIP looks good to me.
> >
> > Best,
> > David
> >
> > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io>
> wrote:
> >
> > > After reading a bit more about it in the Kubernetes case, I think it's
> > > reasonable to do this and be explicit that we're ignoring the value,
> > > just deleting all keys that appear in the file.
> > >
> > > I've updated the KIP wiki page to reflect that:
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > >
> > > And updated my sample PR:
> > > https://github.com/apache/kafka/pull/8184
> > >
> > > If there are no further comments, I'll request a vote in a few days.
> > >
> > > Thanks for the feedback!
> > >
> > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > wrote:
> > > >
> > > > Hi David,
> > > >
> > > > Is the expected behavior that the keys are deleted without checking
> the
> > > values?
> > > >
> > > > Let's say I had this file new.properties:
> > > > a=1
> > > > b=2
> > > >
> > > > And ran:
> > > >
> > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > --entity-type brokers --entity-default \
> > > > --alter --add-config-file new.properties
> > > >
> > > > It seems clear what should happen if I run this immediately:
> > > >
> > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > --entity-type brokers --entity-default \
> > > > --alter --delete-config-file new.properties
> > > >
> > > > (Namely that both a and b would now have no values in the config)
> > > >
> > > > But what if this were run in-between:
> > > >
> > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > --entity-type brokers --entity-default \
> > > > --alter --add-config a=3
> > > >
> > > > Would it be surprising if the key/value pair a=3 was deleted, even
> > > > though the config that is in the file is a=1? Or would that be
> > > > expected?
> > > >
> > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > wrote:
> > > > >
> > > > > Hi Colin,
> > > > >
> > > > > Yes, you're right. This is weird but convenient because you don't
> > have
> > > to
> > > > > duplicate
> > > > > the "keys". I was thinking about the kubernetes API which allows to
> > > create
> > > > > a Pod
> > > > > based on a file and allows to delete it as well with the same
> file. I
> > > have
> > > > > always found
> > > > > this convenient, especially when doing local tests.
> > > > >
> > > > > Best,
> > > > > David
> > > > >
> > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <cm...@apache.org>
> > > wrote:
> > > > >
> > > > > > Hi Aneel,
> > > > > >
> > > > > > Thanks for the KIP. I like the idea.
> > > > > >
> > > > > > You mention that "input from STDIN can be used instead of a file
> on
> > > > > > disk." The example given in the KIP seems to suggest that the
> > > command
> > > > > > defaults to reading from STDIN if no argument is given to
> > > --add-config-file.
> > > > > >
> > > > > > I would argue against this particular command-line pattern. From
> > the
> > > > > > user's point of view, if they mess up and forget to supply an
> > > argument, or
> > > > > > for some reason the parser doesn't treat something like an
> > argument,
> > > the
> > > > > > program will appear to hang in a confusing way.
> > > > > >
> > > > > > Instead, it would be better to follow the traditional UNIX
> pattern
> > > where a
> > > > > > dash indicates that STDIN should be read. So "--add-config-file
> -"
> > > would
> > > > > > indicate that the program should read form STDIN. This would be
> > > difficult
> > > > > > to trigger accidentally, and more in line with the traditional
> > > conventions.
> > > > > >
> > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > I wonder if we should also add a `--delete-config-file` as a
> > > counterpart
> > > > > > of
> > > > > > > `--add-config-file`. It would be a bit weird to use a
> properties
> > > file in
> > > > > > > this case as the values are not necessary but it may be handy
> to
> > > have the
> > > > > > > possibility to remove the configurations which have been set.
> > Have
> > > you
> > > > > > > considered this?
> > > > > >
> > > > > > Hi David,
> > > > > >
> > > > > > That's an interesting idea. However, I think it might be
> confusing
> > > to
> > > > > > users to supply a file, and then have the values supplied in that
> > > file be
> > > > > > ignored. Is there really a case where we need to do this (as
> > > opposed to
> > > > > > creating a file with blank values, or just passing the keys to
> > > > > > --delete-config?
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > David
> > > > > > >
> > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > aneel@confluent.io>
> > > > > > wrote:
> > > > > > >
> > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide to go
> > > ahead
> > > > > > with
> > > > > > > > this KIP.
> > > > > > > >
> > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > aneel@confluent.io>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > I'd like to discuss adding a new argument to
> kafka-configs.sh
> > > > > > > > > (ConfigCommand.scala).
> > > > > > > > >
> > > > > > > > > Recently I've been working on some things that require
> > complex
> > > > > > > > > configurations. I've chosen to represent them as JSON
> strings
> > > in my
> > > > > > > > > server.properties. This works well, and I'm able to update
> > the
> > > > > > > > > configurations by editing server.properties and restarting
> > the
> > > > > > broker.
> > > > > > > > I've
> > > > > > > > > added the ability to dynamically configure them, and that
> > > works well
> > > > > > > > using
> > > > > > > > > the AdminClient. However, when I try to update these
> > > configurations
> > > > > > using
> > > > > > > > > kafka-configs.sh, I run into a problem. My configurations
> > > contain
> > > > > > commas,
> > > > > > > > > and kafka-configs.sh tries to break them up into key/value
> > > pairs at
> > > > > > the
> > > > > > > > > comma boundary.
> > > > > > > > >
> > > > > > > > > I'd like to enable setting these configurations from the
> > > command
> > > > > > line, so
> > > > > > > > > I'm proposing that we add a new option to kafka-configs.sh
> > that
> > > > > > takes a
> > > > > > > > > properties file.
> > > > > > > > >
> > > > > > > > > I've created a KIP for this idea:
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > And a JIRA:
> https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > >
> > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Aneel
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > >
> >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Aneel Nazareth <an...@confluent.io>.
Thanks to David Jacot for his suggestions. Would it be possible for a
committer to have a look at this PR? Thanks!
https://github.com/apache/kafka/pull/8184
On Wed, Apr 1, 2020 at 12:30 PM Aneel Nazareth <an...@confluent.io> wrote:
>
> I have a PR that I think is ready for review here:
> https://github.com/apache/kafka/pull/8184
>
> Feedback would be most welcome!
>
> On Mon, Mar 30, 2020 at 6:57 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > Just as a note about whether we should support "-" as a synonym for STDIN... I agree it's kind of inconsistent.
> >
> > It may not be that big of a deal to drop support for STDIN. A lot of UNIX shells make it easy to create a temporary file in scripts-- for example, you could use a "here document" ( https://en.wikipedia.org/wiki/Here_document )
> >
> > best,
> > Colin
> >
> >
> > On Fri, Mar 27, 2020, at 07:46, Aneel Nazareth wrote:
> > > Update: I have simplified the KIP down to just adding the single new
> > > --add-config-file option. Thanks for your input, everyone!
> > >
> > > On Thu, Mar 26, 2020 at 10:13 AM Aneel Nazareth <an...@confluent.io> wrote:
> > > >
> > > > Hi Kamal,
> > > >
> > > > Thanks for taking a look at this KIP.
> > > >
> > > > Unfortunately the user actually can't pass the arguments on the
> > > > command line using the existing --add-config option if the values are
> > > > complex structures that contain commas. --add-config assumes that
> > > > commas separate distinct configuration properties. There's a
> > > > workaround using square brackets ("[a,b,c]") for simple lists, but it
> > > > doesn't work for things like nested lists or JSON values.
> > > >
> > > > The motivation for allowing STDIN as well as files is to enable
> > > > grep/pipe workflows in scripts without creating a temporary file. I
> > > > don't know if such workflows will end up being common, and hopefully
> > > > someone with a complex enough use case to require it would also be
> > > > familiar with techniques for securely creating and cleaning up
> > > > temporary files.
> > > >
> > > > I'm okay with excluding the option to allow STDIN in the name of
> > > > consistency, if the consensus thinks that's wise. Anyone else have
> > > > opinions on this?
> > > >
> > > > On Thu, Mar 26, 2020 at 9:02 AM Kamal Chandraprakash
> > > > <ka...@gmail.com> wrote:
> > > > >
> > > > > Hi Colin,
> > > > >
> > > > > We should not support STDIN to maintain uniformity across scripts. If the
> > > > > user wants to pass the arguments in command line,
> > > > > they can always use the existing --add-config option.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On Thu, Mar 26, 2020 at 7:20 PM David Jacot <dj...@confluent.io> wrote:
> > > > >
> > > > > > Rajini has made a good point. I don't feel strong for either ways but if
> > > > > > people
> > > > > > are confused by this, it is probably better without it.
> > > > > >
> > > > > > Best,
> > > > > > David
> > > > > >
> > > > > > On Thu, Mar 26, 2020 at 7:23 AM Colin McCabe <cm...@apache.org> wrote:
> > > > > >
> > > > > > > Hi Kamal,
> > > > > > >
> > > > > > > Are you suggesting that we not support STDIN here? I have mixed
> > > > > > feelings.
> > > > > > >
> > > > > > > I think the ideal solution would be to support "-" in these tools
> > > > > > whenever
> > > > > > > a file argument was expected. But that would be a bigger change than
> > > > > > what
> > > > > > > we're talking about here. Maybe you are right and we should keep it
> > > > > > simple
> > > > > > > for now.
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > > > On Wed, Mar 25, 2020, at 01:24, Kamal Chandraprakash wrote:
> > > > > > > > STDIN wasn't standard practice in other scripts like
> > > > > > > > kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> > > > > > > > in which the props file is accepted via consumer.config /
> > > > > > > producer.config /
> > > > > > > > command-config parameter.
> > > > > > > >
> > > > > > > > Shouldn't we have to maintain the uniformity across scripts?
> > > > > > > >
> > > > > > > > On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi Aneel,
> > > > > > > > >
> > > > > > > > > Thanks for the updated KIP. I have made a second pass over it and the
> > > > > > > > > KIP looks good to me.
> > > > > > > > >
> > > > > > > > > Best,
> > > > > > > > > David
> > > > > > > > >
> > > > > > > > > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io>
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > After reading a bit more about it in the Kubernetes case, I think
> > > > > > > it's
> > > > > > > > > > reasonable to do this and be explicit that we're ignoring the
> > > > > > value,
> > > > > > > > > > just deleting all keys that appear in the file.
> > > > > > > > > >
> > > > > > > > > > I've updated the KIP wiki page to reflect that:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > >
> > > > > > > > > > And updated my sample PR:
> > > > > > > > > > https://github.com/apache/kafka/pull/8184
> > > > > > > > > >
> > > > > > > > > > If there are no further comments, I'll request a vote in a few
> > > > > > days.
> > > > > > > > > >
> > > > > > > > > > Thanks for the feedback!
> > > > > > > > > >
> > > > > > > > > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi David,
> > > > > > > > > > >
> > > > > > > > > > > Is the expected behavior that the keys are deleted without
> > > > > > > checking the
> > > > > > > > > > values?
> > > > > > > > > > >
> > > > > > > > > > > Let's say I had this file new.properties:
> > > > > > > > > > > a=1
> > > > > > > > > > > b=2
> > > > > > > > > > >
> > > > > > > > > > > And ran:
> > > > > > > > > > >
> > > > > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > > > > --alter --add-config-file new.properties
> > > > > > > > > > >
> > > > > > > > > > > It seems clear what should happen if I run this immediately:
> > > > > > > > > > >
> > > > > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > > > > --alter --delete-config-file new.properties
> > > > > > > > > > >
> > > > > > > > > > > (Namely that both a and b would now have no values in the config)
> > > > > > > > > > >
> > > > > > > > > > > But what if this were run in-between:
> > > > > > > > > > >
> > > > > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > > > > --alter --add-config a=3
> > > > > > > > > > >
> > > > > > > > > > > Would it be surprising if the key/value pair a=3 was deleted,
> > > > > > even
> > > > > > > > > > > though the config that is in the file is a=1? Or would that be
> > > > > > > > > > > expected?
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Colin,
> > > > > > > > > > > >
> > > > > > > > > > > > Yes, you're right. This is weird but convenient because you
> > > > > > don't
> > > > > > > > > have
> > > > > > > > > > to
> > > > > > > > > > > > duplicate
> > > > > > > > > > > > the "keys". I was thinking about the kubernetes API which
> > > > > > allows
> > > > > > > to
> > > > > > > > > > create
> > > > > > > > > > > > a Pod
> > > > > > > > > > > > based on a file and allows to delete it as well with the same
> > > > > > > file. I
> > > > > > > > > > have
> > > > > > > > > > > > always found
> > > > > > > > > > > > this convenient, especially when doing local tests.
> > > > > > > > > > > >
> > > > > > > > > > > > Best,
> > > > > > > > > > > > David
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <
> > > > > > cmccabe@apache.org>
> > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > Hi Aneel,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks for the KIP. I like the idea.
> > > > > > > > > > > > >
> > > > > > > > > > > > > You mention that "input from STDIN can be used instead of a
> > > > > > > file on
> > > > > > > > > > > > > disk." The example given in the KIP seems to suggest that
> > > > > > the
> > > > > > > > > > command
> > > > > > > > > > > > > defaults to reading from STDIN if no argument is given to
> > > > > > > > > > --add-config-file.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I would argue against this particular command-line pattern.
> > > > > > > From
> > > > > > > > > the
> > > > > > > > > > > > > user's point of view, if they mess up and forget to supply an
> > > > > > > > > > argument, or
> > > > > > > > > > > > > for some reason the parser doesn't treat something like an
> > > > > > > > > argument,
> > > > > > > > > > the
> > > > > > > > > > > > > program will appear to hang in a confusing way.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Instead, it would be better to follow the traditional UNIX
> > > > > > > pattern
> > > > > > > > > > where a
> > > > > > > > > > > > > dash indicates that STDIN should be read. So
> > > > > > > "--add-config-file -"
> > > > > > > > > > would
> > > > > > > > > > > > > indicate that the program should read form STDIN. This would
> > > > > > > be
> > > > > > > > > > difficult
> > > > > > > > > > > > > to trigger accidentally, and more in line with the
> > > > > > traditional
> > > > > > > > > > conventions.
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > > > > > > > > I wonder if we should also add a `--delete-config-file` as
> > > > > > a
> > > > > > > > > > counterpart
> > > > > > > > > > > > > of
> > > > > > > > > > > > > > `--add-config-file`. It would be a bit weird to use a
> > > > > > > properties
> > > > > > > > > > file in
> > > > > > > > > > > > > > this case as the values are not necessary but it may be
> > > > > > > handy to
> > > > > > > > > > have the
> > > > > > > > > > > > > > possibility to remove the configurations which have been
> > > > > > set.
> > > > > > > > > Have
> > > > > > > > > > you
> > > > > > > > > > > > > > considered this?
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hi David,
> > > > > > > > > > > > >
> > > > > > > > > > > > > That's an interesting idea. However, I think it might be
> > > > > > > confusing
> > > > > > > > > > to
> > > > > > > > > > > > > users to supply a file, and then have the values supplied in
> > > > > > > that
> > > > > > > > > > file be
> > > > > > > > > > > > > ignored. Is there really a case where we need to do this (as
> > > > > > > > > > opposed to
> > > > > > > > > > > > > creating a file with blank values, or just passing the keys
> > > > > > to
> > > > > > > > > > > > > --delete-config?
> > > > > > > > > > > > >
> > > > > > > > > > > > > best,
> > > > > > > > > > > > > Colin
> > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > David
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > > > > > > > > aneel@confluent.io>
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide
> > > > > > to
> > > > > > > go
> > > > > > > > > > ahead
> > > > > > > > > > > > > with
> > > > > > > > > > > > > > > this KIP.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > > > > > > > > aneel@confluent.io>
> > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I'd like to discuss adding a new argument to
> > > > > > > kafka-configs.sh
> > > > > > > > > > > > > > > > (ConfigCommand.scala).
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Recently I've been working on some things that require
> > > > > > > > > complex
> > > > > > > > > > > > > > > > configurations. I've chosen to represent them as JSON
> > > > > > > strings
> > > > > > > > > > in my
> > > > > > > > > > > > > > > > server.properties. This works well, and I'm able to
> > > > > > > update
> > > > > > > > > the
> > > > > > > > > > > > > > > > configurations by editing server.properties and
> > > > > > > restarting
> > > > > > > > > the
> > > > > > > > > > > > > broker.
> > > > > > > > > > > > > > > I've
> > > > > > > > > > > > > > > > added the ability to dynamically configure them, and
> > > > > > that
> > > > > > > > > > works well
> > > > > > > > > > > > > > > using
> > > > > > > > > > > > > > > > the AdminClient. However, when I try to update these
> > > > > > > > > > configurations
> > > > > > > > > > > > > using
> > > > > > > > > > > > > > > > kafka-configs.sh, I run into a problem. My
> > > > > > configurations
> > > > > > > > > > contain
> > > > > > > > > > > > > commas,
> > > > > > > > > > > > > > > > and kafka-configs.sh tries to break them up into
> > > > > > > key/value
> > > > > > > > > > pairs at
> > > > > > > > > > > > > the
> > > > > > > > > > > > > > > > comma boundary.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I'd like to enable setting these configurations from
> > > > > > the
> > > > > > > > > > command
> > > > > > > > > > > > > line, so
> > > > > > > > > > > > > > > > I'm proposing that we add a new option to
> > > > > > > kafka-configs.sh
> > > > > > > > > that
> > > > > > > > > > > > > takes a
> > > > > > > > > > > > > > > > properties file.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I've created a KIP for this idea:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > > > > > > > > And a JIRA:
> > > > > > > https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > > > > Aneel
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > >
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Aneel Nazareth <an...@confluent.io>.
I have a PR that I think is ready for review here:
https://github.com/apache/kafka/pull/8184
Feedback would be most welcome!
On Mon, Mar 30, 2020 at 6:57 PM Colin McCabe <cm...@apache.org> wrote:
>
> Just as a note about whether we should support "-" as a synonym for STDIN... I agree it's kind of inconsistent.
>
> It may not be that big of a deal to drop support for STDIN. A lot of UNIX shells make it easy to create a temporary file in scripts-- for example, you could use a "here document" ( https://en.wikipedia.org/wiki/Here_document )
>
> best,
> Colin
>
>
> On Fri, Mar 27, 2020, at 07:46, Aneel Nazareth wrote:
> > Update: I have simplified the KIP down to just adding the single new
> > --add-config-file option. Thanks for your input, everyone!
> >
> > On Thu, Mar 26, 2020 at 10:13 AM Aneel Nazareth <an...@confluent.io> wrote:
> > >
> > > Hi Kamal,
> > >
> > > Thanks for taking a look at this KIP.
> > >
> > > Unfortunately the user actually can't pass the arguments on the
> > > command line using the existing --add-config option if the values are
> > > complex structures that contain commas. --add-config assumes that
> > > commas separate distinct configuration properties. There's a
> > > workaround using square brackets ("[a,b,c]") for simple lists, but it
> > > doesn't work for things like nested lists or JSON values.
> > >
> > > The motivation for allowing STDIN as well as files is to enable
> > > grep/pipe workflows in scripts without creating a temporary file. I
> > > don't know if such workflows will end up being common, and hopefully
> > > someone with a complex enough use case to require it would also be
> > > familiar with techniques for securely creating and cleaning up
> > > temporary files.
> > >
> > > I'm okay with excluding the option to allow STDIN in the name of
> > > consistency, if the consensus thinks that's wise. Anyone else have
> > > opinions on this?
> > >
> > > On Thu, Mar 26, 2020 at 9:02 AM Kamal Chandraprakash
> > > <ka...@gmail.com> wrote:
> > > >
> > > > Hi Colin,
> > > >
> > > > We should not support STDIN to maintain uniformity across scripts. If the
> > > > user wants to pass the arguments in command line,
> > > > they can always use the existing --add-config option.
> > > >
> > > >
> > > >
> > > >
> > > > On Thu, Mar 26, 2020 at 7:20 PM David Jacot <dj...@confluent.io> wrote:
> > > >
> > > > > Rajini has made a good point. I don't feel strong for either ways but if
> > > > > people
> > > > > are confused by this, it is probably better without it.
> > > > >
> > > > > Best,
> > > > > David
> > > > >
> > > > > On Thu, Mar 26, 2020 at 7:23 AM Colin McCabe <cm...@apache.org> wrote:
> > > > >
> > > > > > Hi Kamal,
> > > > > >
> > > > > > Are you suggesting that we not support STDIN here? I have mixed
> > > > > feelings.
> > > > > >
> > > > > > I think the ideal solution would be to support "-" in these tools
> > > > > whenever
> > > > > > a file argument was expected. But that would be a bigger change than
> > > > > what
> > > > > > we're talking about here. Maybe you are right and we should keep it
> > > > > simple
> > > > > > for now.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > > On Wed, Mar 25, 2020, at 01:24, Kamal Chandraprakash wrote:
> > > > > > > STDIN wasn't standard practice in other scripts like
> > > > > > > kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> > > > > > > in which the props file is accepted via consumer.config /
> > > > > > producer.config /
> > > > > > > command-config parameter.
> > > > > > >
> > > > > > > Shouldn't we have to maintain the uniformity across scripts?
> > > > > > >
> > > > > > > On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io>
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi Aneel,
> > > > > > > >
> > > > > > > > Thanks for the updated KIP. I have made a second pass over it and the
> > > > > > > > KIP looks good to me.
> > > > > > > >
> > > > > > > > Best,
> > > > > > > > David
> > > > > > > >
> > > > > > > > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > After reading a bit more about it in the Kubernetes case, I think
> > > > > > it's
> > > > > > > > > reasonable to do this and be explicit that we're ignoring the
> > > > > value,
> > > > > > > > > just deleting all keys that appear in the file.
> > > > > > > > >
> > > > > > > > > I've updated the KIP wiki page to reflect that:
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > >
> > > > > > > > > And updated my sample PR:
> > > > > > > > > https://github.com/apache/kafka/pull/8184
> > > > > > > > >
> > > > > > > > > If there are no further comments, I'll request a vote in a few
> > > > > days.
> > > > > > > > >
> > > > > > > > > Thanks for the feedback!
> > > > > > > > >
> > > > > > > > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hi David,
> > > > > > > > > >
> > > > > > > > > > Is the expected behavior that the keys are deleted without
> > > > > > checking the
> > > > > > > > > values?
> > > > > > > > > >
> > > > > > > > > > Let's say I had this file new.properties:
> > > > > > > > > > a=1
> > > > > > > > > > b=2
> > > > > > > > > >
> > > > > > > > > > And ran:
> > > > > > > > > >
> > > > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > > > --alter --add-config-file new.properties
> > > > > > > > > >
> > > > > > > > > > It seems clear what should happen if I run this immediately:
> > > > > > > > > >
> > > > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > > > --alter --delete-config-file new.properties
> > > > > > > > > >
> > > > > > > > > > (Namely that both a and b would now have no values in the config)
> > > > > > > > > >
> > > > > > > > > > But what if this were run in-between:
> > > > > > > > > >
> > > > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > > > --alter --add-config a=3
> > > > > > > > > >
> > > > > > > > > > Would it be surprising if the key/value pair a=3 was deleted,
> > > > > even
> > > > > > > > > > though the config that is in the file is a=1? Or would that be
> > > > > > > > > > expected?
> > > > > > > > > >
> > > > > > > > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Colin,
> > > > > > > > > > >
> > > > > > > > > > > Yes, you're right. This is weird but convenient because you
> > > > > don't
> > > > > > > > have
> > > > > > > > > to
> > > > > > > > > > > duplicate
> > > > > > > > > > > the "keys". I was thinking about the kubernetes API which
> > > > > allows
> > > > > > to
> > > > > > > > > create
> > > > > > > > > > > a Pod
> > > > > > > > > > > based on a file and allows to delete it as well with the same
> > > > > > file. I
> > > > > > > > > have
> > > > > > > > > > > always found
> > > > > > > > > > > this convenient, especially when doing local tests.
> > > > > > > > > > >
> > > > > > > > > > > Best,
> > > > > > > > > > > David
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <
> > > > > cmccabe@apache.org>
> > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Hi Aneel,
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks for the KIP. I like the idea.
> > > > > > > > > > > >
> > > > > > > > > > > > You mention that "input from STDIN can be used instead of a
> > > > > > file on
> > > > > > > > > > > > disk." The example given in the KIP seems to suggest that
> > > > > the
> > > > > > > > > command
> > > > > > > > > > > > defaults to reading from STDIN if no argument is given to
> > > > > > > > > --add-config-file.
> > > > > > > > > > > >
> > > > > > > > > > > > I would argue against this particular command-line pattern.
> > > > > > From
> > > > > > > > the
> > > > > > > > > > > > user's point of view, if they mess up and forget to supply an
> > > > > > > > > argument, or
> > > > > > > > > > > > for some reason the parser doesn't treat something like an
> > > > > > > > argument,
> > > > > > > > > the
> > > > > > > > > > > > program will appear to hang in a confusing way.
> > > > > > > > > > > >
> > > > > > > > > > > > Instead, it would be better to follow the traditional UNIX
> > > > > > pattern
> > > > > > > > > where a
> > > > > > > > > > > > dash indicates that STDIN should be read. So
> > > > > > "--add-config-file -"
> > > > > > > > > would
> > > > > > > > > > > > indicate that the program should read form STDIN. This would
> > > > > > be
> > > > > > > > > difficult
> > > > > > > > > > > > to trigger accidentally, and more in line with the
> > > > > traditional
> > > > > > > > > conventions.
> > > > > > > > > > > >
> > > > > > > > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > > > > > > > I wonder if we should also add a `--delete-config-file` as
> > > > > a
> > > > > > > > > counterpart
> > > > > > > > > > > > of
> > > > > > > > > > > > > `--add-config-file`. It would be a bit weird to use a
> > > > > > properties
> > > > > > > > > file in
> > > > > > > > > > > > > this case as the values are not necessary but it may be
> > > > > > handy to
> > > > > > > > > have the
> > > > > > > > > > > > > possibility to remove the configurations which have been
> > > > > set.
> > > > > > > > Have
> > > > > > > > > you
> > > > > > > > > > > > > considered this?
> > > > > > > > > > > >
> > > > > > > > > > > > Hi David,
> > > > > > > > > > > >
> > > > > > > > > > > > That's an interesting idea. However, I think it might be
> > > > > > confusing
> > > > > > > > > to
> > > > > > > > > > > > users to supply a file, and then have the values supplied in
> > > > > > that
> > > > > > > > > file be
> > > > > > > > > > > > ignored. Is there really a case where we need to do this (as
> > > > > > > > > opposed to
> > > > > > > > > > > > creating a file with blank values, or just passing the keys
> > > > > to
> > > > > > > > > > > > --delete-config?
> > > > > > > > > > > >
> > > > > > > > > > > > best,
> > > > > > > > > > > > Colin
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > David
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > > > > > > > aneel@confluent.io>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide
> > > > > to
> > > > > > go
> > > > > > > > > ahead
> > > > > > > > > > > > with
> > > > > > > > > > > > > > this KIP.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > > > > > > > aneel@confluent.io>
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I'd like to discuss adding a new argument to
> > > > > > kafka-configs.sh
> > > > > > > > > > > > > > > (ConfigCommand.scala).
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Recently I've been working on some things that require
> > > > > > > > complex
> > > > > > > > > > > > > > > configurations. I've chosen to represent them as JSON
> > > > > > strings
> > > > > > > > > in my
> > > > > > > > > > > > > > > server.properties. This works well, and I'm able to
> > > > > > update
> > > > > > > > the
> > > > > > > > > > > > > > > configurations by editing server.properties and
> > > > > > restarting
> > > > > > > > the
> > > > > > > > > > > > broker.
> > > > > > > > > > > > > > I've
> > > > > > > > > > > > > > > added the ability to dynamically configure them, and
> > > > > that
> > > > > > > > > works well
> > > > > > > > > > > > > > using
> > > > > > > > > > > > > > > the AdminClient. However, when I try to update these
> > > > > > > > > configurations
> > > > > > > > > > > > using
> > > > > > > > > > > > > > > kafka-configs.sh, I run into a problem. My
> > > > > configurations
> > > > > > > > > contain
> > > > > > > > > > > > commas,
> > > > > > > > > > > > > > > and kafka-configs.sh tries to break them up into
> > > > > > key/value
> > > > > > > > > pairs at
> > > > > > > > > > > > the
> > > > > > > > > > > > > > > comma boundary.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I'd like to enable setting these configurations from
> > > > > the
> > > > > > > > > command
> > > > > > > > > > > > line, so
> > > > > > > > > > > > > > > I'm proposing that we add a new option to
> > > > > > kafka-configs.sh
> > > > > > > > that
> > > > > > > > > > > > takes a
> > > > > > > > > > > > > > > properties file.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I've created a KIP for this idea:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > > > > > > > And a JIRA:
> > > > > > https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > > > Aneel
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> >
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Colin McCabe <cm...@apache.org>.
Just as a note about whether we should support "-" as a synonym for STDIN... I agree it's kind of inconsistent.
It may not be that big of a deal to drop support for STDIN. A lot of UNIX shells make it easy to create a temporary file in scripts-- for example, you could use a "here document" ( https://en.wikipedia.org/wiki/Here_document )
best,
Colin
On Fri, Mar 27, 2020, at 07:46, Aneel Nazareth wrote:
> Update: I have simplified the KIP down to just adding the single new
> --add-config-file option. Thanks for your input, everyone!
>
> On Thu, Mar 26, 2020 at 10:13 AM Aneel Nazareth <an...@confluent.io> wrote:
> >
> > Hi Kamal,
> >
> > Thanks for taking a look at this KIP.
> >
> > Unfortunately the user actually can't pass the arguments on the
> > command line using the existing --add-config option if the values are
> > complex structures that contain commas. --add-config assumes that
> > commas separate distinct configuration properties. There's a
> > workaround using square brackets ("[a,b,c]") for simple lists, but it
> > doesn't work for things like nested lists or JSON values.
> >
> > The motivation for allowing STDIN as well as files is to enable
> > grep/pipe workflows in scripts without creating a temporary file. I
> > don't know if such workflows will end up being common, and hopefully
> > someone with a complex enough use case to require it would also be
> > familiar with techniques for securely creating and cleaning up
> > temporary files.
> >
> > I'm okay with excluding the option to allow STDIN in the name of
> > consistency, if the consensus thinks that's wise. Anyone else have
> > opinions on this?
> >
> > On Thu, Mar 26, 2020 at 9:02 AM Kamal Chandraprakash
> > <ka...@gmail.com> wrote:
> > >
> > > Hi Colin,
> > >
> > > We should not support STDIN to maintain uniformity across scripts. If the
> > > user wants to pass the arguments in command line,
> > > they can always use the existing --add-config option.
> > >
> > >
> > >
> > >
> > > On Thu, Mar 26, 2020 at 7:20 PM David Jacot <dj...@confluent.io> wrote:
> > >
> > > > Rajini has made a good point. I don't feel strong for either ways but if
> > > > people
> > > > are confused by this, it is probably better without it.
> > > >
> > > > Best,
> > > > David
> > > >
> > > > On Thu, Mar 26, 2020 at 7:23 AM Colin McCabe <cm...@apache.org> wrote:
> > > >
> > > > > Hi Kamal,
> > > > >
> > > > > Are you suggesting that we not support STDIN here? I have mixed
> > > > feelings.
> > > > >
> > > > > I think the ideal solution would be to support "-" in these tools
> > > > whenever
> > > > > a file argument was expected. But that would be a bigger change than
> > > > what
> > > > > we're talking about here. Maybe you are right and we should keep it
> > > > simple
> > > > > for now.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > > On Wed, Mar 25, 2020, at 01:24, Kamal Chandraprakash wrote:
> > > > > > STDIN wasn't standard practice in other scripts like
> > > > > > kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> > > > > > in which the props file is accepted via consumer.config /
> > > > > producer.config /
> > > > > > command-config parameter.
> > > > > >
> > > > > > Shouldn't we have to maintain the uniformity across scripts?
> > > > > >
> > > > > > On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io>
> > > > wrote:
> > > > > >
> > > > > > > Hi Aneel,
> > > > > > >
> > > > > > > Thanks for the updated KIP. I have made a second pass over it and the
> > > > > > > KIP looks good to me.
> > > > > > >
> > > > > > > Best,
> > > > > > > David
> > > > > > >
> > > > > > > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io>
> > > > > wrote:
> > > > > > >
> > > > > > > > After reading a bit more about it in the Kubernetes case, I think
> > > > > it's
> > > > > > > > reasonable to do this and be explicit that we're ignoring the
> > > > value,
> > > > > > > > just deleting all keys that appear in the file.
> > > > > > > >
> > > > > > > > I've updated the KIP wiki page to reflect that:
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > >
> > > > > > > > And updated my sample PR:
> > > > > > > > https://github.com/apache/kafka/pull/8184
> > > > > > > >
> > > > > > > > If there are no further comments, I'll request a vote in a few
> > > > days.
> > > > > > > >
> > > > > > > > Thanks for the feedback!
> > > > > > > >
> > > > > > > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hi David,
> > > > > > > > >
> > > > > > > > > Is the expected behavior that the keys are deleted without
> > > > > checking the
> > > > > > > > values?
> > > > > > > > >
> > > > > > > > > Let's say I had this file new.properties:
> > > > > > > > > a=1
> > > > > > > > > b=2
> > > > > > > > >
> > > > > > > > > And ran:
> > > > > > > > >
> > > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > > --alter --add-config-file new.properties
> > > > > > > > >
> > > > > > > > > It seems clear what should happen if I run this immediately:
> > > > > > > > >
> > > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > > --alter --delete-config-file new.properties
> > > > > > > > >
> > > > > > > > > (Namely that both a and b would now have no values in the config)
> > > > > > > > >
> > > > > > > > > But what if this were run in-between:
> > > > > > > > >
> > > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > > --alter --add-config a=3
> > > > > > > > >
> > > > > > > > > Would it be surprising if the key/value pair a=3 was deleted,
> > > > even
> > > > > > > > > though the config that is in the file is a=1? Or would that be
> > > > > > > > > expected?
> > > > > > > > >
> > > > > > > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Colin,
> > > > > > > > > >
> > > > > > > > > > Yes, you're right. This is weird but convenient because you
> > > > don't
> > > > > > > have
> > > > > > > > to
> > > > > > > > > > duplicate
> > > > > > > > > > the "keys". I was thinking about the kubernetes API which
> > > > allows
> > > > > to
> > > > > > > > create
> > > > > > > > > > a Pod
> > > > > > > > > > based on a file and allows to delete it as well with the same
> > > > > file. I
> > > > > > > > have
> > > > > > > > > > always found
> > > > > > > > > > this convenient, especially when doing local tests.
> > > > > > > > > >
> > > > > > > > > > Best,
> > > > > > > > > > David
> > > > > > > > > >
> > > > > > > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <
> > > > cmccabe@apache.org>
> > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi Aneel,
> > > > > > > > > > >
> > > > > > > > > > > Thanks for the KIP. I like the idea.
> > > > > > > > > > >
> > > > > > > > > > > You mention that "input from STDIN can be used instead of a
> > > > > file on
> > > > > > > > > > > disk." The example given in the KIP seems to suggest that
> > > > the
> > > > > > > > command
> > > > > > > > > > > defaults to reading from STDIN if no argument is given to
> > > > > > > > --add-config-file.
> > > > > > > > > > >
> > > > > > > > > > > I would argue against this particular command-line pattern.
> > > > > From
> > > > > > > the
> > > > > > > > > > > user's point of view, if they mess up and forget to supply an
> > > > > > > > argument, or
> > > > > > > > > > > for some reason the parser doesn't treat something like an
> > > > > > > argument,
> > > > > > > > the
> > > > > > > > > > > program will appear to hang in a confusing way.
> > > > > > > > > > >
> > > > > > > > > > > Instead, it would be better to follow the traditional UNIX
> > > > > pattern
> > > > > > > > where a
> > > > > > > > > > > dash indicates that STDIN should be read. So
> > > > > "--add-config-file -"
> > > > > > > > would
> > > > > > > > > > > indicate that the program should read form STDIN. This would
> > > > > be
> > > > > > > > difficult
> > > > > > > > > > > to trigger accidentally, and more in line with the
> > > > traditional
> > > > > > > > conventions.
> > > > > > > > > > >
> > > > > > > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > > > > > > I wonder if we should also add a `--delete-config-file` as
> > > > a
> > > > > > > > counterpart
> > > > > > > > > > > of
> > > > > > > > > > > > `--add-config-file`. It would be a bit weird to use a
> > > > > properties
> > > > > > > > file in
> > > > > > > > > > > > this case as the values are not necessary but it may be
> > > > > handy to
> > > > > > > > have the
> > > > > > > > > > > > possibility to remove the configurations which have been
> > > > set.
> > > > > > > Have
> > > > > > > > you
> > > > > > > > > > > > considered this?
> > > > > > > > > > >
> > > > > > > > > > > Hi David,
> > > > > > > > > > >
> > > > > > > > > > > That's an interesting idea. However, I think it might be
> > > > > confusing
> > > > > > > > to
> > > > > > > > > > > users to supply a file, and then have the values supplied in
> > > > > that
> > > > > > > > file be
> > > > > > > > > > > ignored. Is there really a case where we need to do this (as
> > > > > > > > opposed to
> > > > > > > > > > > creating a file with blank values, or just passing the keys
> > > > to
> > > > > > > > > > > --delete-config?
> > > > > > > > > > >
> > > > > > > > > > > best,
> > > > > > > > > > > Colin
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > David
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > > > > > > aneel@confluent.io>
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide
> > > > to
> > > > > go
> > > > > > > > ahead
> > > > > > > > > > > with
> > > > > > > > > > > > > this KIP.
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > > > > > > aneel@confluent.io>
> > > > > > > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I'd like to discuss adding a new argument to
> > > > > kafka-configs.sh
> > > > > > > > > > > > > > (ConfigCommand.scala).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Recently I've been working on some things that require
> > > > > > > complex
> > > > > > > > > > > > > > configurations. I've chosen to represent them as JSON
> > > > > strings
> > > > > > > > in my
> > > > > > > > > > > > > > server.properties. This works well, and I'm able to
> > > > > update
> > > > > > > the
> > > > > > > > > > > > > > configurations by editing server.properties and
> > > > > restarting
> > > > > > > the
> > > > > > > > > > > broker.
> > > > > > > > > > > > > I've
> > > > > > > > > > > > > > added the ability to dynamically configure them, and
> > > > that
> > > > > > > > works well
> > > > > > > > > > > > > using
> > > > > > > > > > > > > > the AdminClient. However, when I try to update these
> > > > > > > > configurations
> > > > > > > > > > > using
> > > > > > > > > > > > > > kafka-configs.sh, I run into a problem. My
> > > > configurations
> > > > > > > > contain
> > > > > > > > > > > commas,
> > > > > > > > > > > > > > and kafka-configs.sh tries to break them up into
> > > > > key/value
> > > > > > > > pairs at
> > > > > > > > > > > the
> > > > > > > > > > > > > > comma boundary.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I'd like to enable setting these configurations from
> > > > the
> > > > > > > > command
> > > > > > > > > > > line, so
> > > > > > > > > > > > > > I'm proposing that we add a new option to
> > > > > kafka-configs.sh
> > > > > > > that
> > > > > > > > > > > takes a
> > > > > > > > > > > > > > properties file.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I've created a KIP for this idea:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > > > > > > And a JIRA:
> > > > > https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > > Aneel
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Aneel Nazareth <an...@confluent.io>.
Update: I have simplified the KIP down to just adding the single new
--add-config-file option. Thanks for your input, everyone!
On Thu, Mar 26, 2020 at 10:13 AM Aneel Nazareth <an...@confluent.io> wrote:
>
> Hi Kamal,
>
> Thanks for taking a look at this KIP.
>
> Unfortunately the user actually can't pass the arguments on the
> command line using the existing --add-config option if the values are
> complex structures that contain commas. --add-config assumes that
> commas separate distinct configuration properties. There's a
> workaround using square brackets ("[a,b,c]") for simple lists, but it
> doesn't work for things like nested lists or JSON values.
>
> The motivation for allowing STDIN as well as files is to enable
> grep/pipe workflows in scripts without creating a temporary file. I
> don't know if such workflows will end up being common, and hopefully
> someone with a complex enough use case to require it would also be
> familiar with techniques for securely creating and cleaning up
> temporary files.
>
> I'm okay with excluding the option to allow STDIN in the name of
> consistency, if the consensus thinks that's wise. Anyone else have
> opinions on this?
>
> On Thu, Mar 26, 2020 at 9:02 AM Kamal Chandraprakash
> <ka...@gmail.com> wrote:
> >
> > Hi Colin,
> >
> > We should not support STDIN to maintain uniformity across scripts. If the
> > user wants to pass the arguments in command line,
> > they can always use the existing --add-config option.
> >
> >
> >
> >
> > On Thu, Mar 26, 2020 at 7:20 PM David Jacot <dj...@confluent.io> wrote:
> >
> > > Rajini has made a good point. I don't feel strong for either ways but if
> > > people
> > > are confused by this, it is probably better without it.
> > >
> > > Best,
> > > David
> > >
> > > On Thu, Mar 26, 2020 at 7:23 AM Colin McCabe <cm...@apache.org> wrote:
> > >
> > > > Hi Kamal,
> > > >
> > > > Are you suggesting that we not support STDIN here? I have mixed
> > > feelings.
> > > >
> > > > I think the ideal solution would be to support "-" in these tools
> > > whenever
> > > > a file argument was expected. But that would be a bigger change than
> > > what
> > > > we're talking about here. Maybe you are right and we should keep it
> > > simple
> > > > for now.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > > On Wed, Mar 25, 2020, at 01:24, Kamal Chandraprakash wrote:
> > > > > STDIN wasn't standard practice in other scripts like
> > > > > kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> > > > > in which the props file is accepted via consumer.config /
> > > > producer.config /
> > > > > command-config parameter.
> > > > >
> > > > > Shouldn't we have to maintain the uniformity across scripts?
> > > > >
> > > > > On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io>
> > > wrote:
> > > > >
> > > > > > Hi Aneel,
> > > > > >
> > > > > > Thanks for the updated KIP. I have made a second pass over it and the
> > > > > > KIP looks good to me.
> > > > > >
> > > > > > Best,
> > > > > > David
> > > > > >
> > > > > > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io>
> > > > wrote:
> > > > > >
> > > > > > > After reading a bit more about it in the Kubernetes case, I think
> > > > it's
> > > > > > > reasonable to do this and be explicit that we're ignoring the
> > > value,
> > > > > > > just deleting all keys that appear in the file.
> > > > > > >
> > > > > > > I've updated the KIP wiki page to reflect that:
> > > > > > >
> > > > > > >
> > > > > >
> > > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > >
> > > > > > > And updated my sample PR:
> > > > > > > https://github.com/apache/kafka/pull/8184
> > > > > > >
> > > > > > > If there are no further comments, I'll request a vote in a few
> > > days.
> > > > > > >
> > > > > > > Thanks for the feedback!
> > > > > > >
> > > > > > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > Hi David,
> > > > > > > >
> > > > > > > > Is the expected behavior that the keys are deleted without
> > > > checking the
> > > > > > > values?
> > > > > > > >
> > > > > > > > Let's say I had this file new.properties:
> > > > > > > > a=1
> > > > > > > > b=2
> > > > > > > >
> > > > > > > > And ran:
> > > > > > > >
> > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > --alter --add-config-file new.properties
> > > > > > > >
> > > > > > > > It seems clear what should happen if I run this immediately:
> > > > > > > >
> > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > --alter --delete-config-file new.properties
> > > > > > > >
> > > > > > > > (Namely that both a and b would now have no values in the config)
> > > > > > > >
> > > > > > > > But what if this were run in-between:
> > > > > > > >
> > > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > > --entity-type brokers --entity-default \
> > > > > > > > --alter --add-config a=3
> > > > > > > >
> > > > > > > > Would it be surprising if the key/value pair a=3 was deleted,
> > > even
> > > > > > > > though the config that is in the file is a=1? Or would that be
> > > > > > > > expected?
> > > > > > > >
> > > > > > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hi Colin,
> > > > > > > > >
> > > > > > > > > Yes, you're right. This is weird but convenient because you
> > > don't
> > > > > > have
> > > > > > > to
> > > > > > > > > duplicate
> > > > > > > > > the "keys". I was thinking about the kubernetes API which
> > > allows
> > > > to
> > > > > > > create
> > > > > > > > > a Pod
> > > > > > > > > based on a file and allows to delete it as well with the same
> > > > file. I
> > > > > > > have
> > > > > > > > > always found
> > > > > > > > > this convenient, especially when doing local tests.
> > > > > > > > >
> > > > > > > > > Best,
> > > > > > > > > David
> > > > > > > > >
> > > > > > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <
> > > cmccabe@apache.org>
> > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi Aneel,
> > > > > > > > > >
> > > > > > > > > > Thanks for the KIP. I like the idea.
> > > > > > > > > >
> > > > > > > > > > You mention that "input from STDIN can be used instead of a
> > > > file on
> > > > > > > > > > disk." The example given in the KIP seems to suggest that
> > > the
> > > > > > > command
> > > > > > > > > > defaults to reading from STDIN if no argument is given to
> > > > > > > --add-config-file.
> > > > > > > > > >
> > > > > > > > > > I would argue against this particular command-line pattern.
> > > > From
> > > > > > the
> > > > > > > > > > user's point of view, if they mess up and forget to supply an
> > > > > > > argument, or
> > > > > > > > > > for some reason the parser doesn't treat something like an
> > > > > > argument,
> > > > > > > the
> > > > > > > > > > program will appear to hang in a confusing way.
> > > > > > > > > >
> > > > > > > > > > Instead, it would be better to follow the traditional UNIX
> > > > pattern
> > > > > > > where a
> > > > > > > > > > dash indicates that STDIN should be read. So
> > > > "--add-config-file -"
> > > > > > > would
> > > > > > > > > > indicate that the program should read form STDIN. This would
> > > > be
> > > > > > > difficult
> > > > > > > > > > to trigger accidentally, and more in line with the
> > > traditional
> > > > > > > conventions.
> > > > > > > > > >
> > > > > > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > > > > > I wonder if we should also add a `--delete-config-file` as
> > > a
> > > > > > > counterpart
> > > > > > > > > > of
> > > > > > > > > > > `--add-config-file`. It would be a bit weird to use a
> > > > properties
> > > > > > > file in
> > > > > > > > > > > this case as the values are not necessary but it may be
> > > > handy to
> > > > > > > have the
> > > > > > > > > > > possibility to remove the configurations which have been
> > > set.
> > > > > > Have
> > > > > > > you
> > > > > > > > > > > considered this?
> > > > > > > > > >
> > > > > > > > > > Hi David,
> > > > > > > > > >
> > > > > > > > > > That's an interesting idea. However, I think it might be
> > > > confusing
> > > > > > > to
> > > > > > > > > > users to supply a file, and then have the values supplied in
> > > > that
> > > > > > > file be
> > > > > > > > > > ignored. Is there really a case where we need to do this (as
> > > > > > > opposed to
> > > > > > > > > > creating a file with blank values, or just passing the keys
> > > to
> > > > > > > > > > --delete-config?
> > > > > > > > > >
> > > > > > > > > > best,
> > > > > > > > > > Colin
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > David
> > > > > > > > > > >
> > > > > > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > > > > > aneel@confluent.io>
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide
> > > to
> > > > go
> > > > > > > ahead
> > > > > > > > > > with
> > > > > > > > > > > > this KIP.
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > > > > > aneel@confluent.io>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > >
> > > > > > > > > > > > > I'd like to discuss adding a new argument to
> > > > kafka-configs.sh
> > > > > > > > > > > > > (ConfigCommand.scala).
> > > > > > > > > > > > >
> > > > > > > > > > > > > Recently I've been working on some things that require
> > > > > > complex
> > > > > > > > > > > > > configurations. I've chosen to represent them as JSON
> > > > strings
> > > > > > > in my
> > > > > > > > > > > > > server.properties. This works well, and I'm able to
> > > > update
> > > > > > the
> > > > > > > > > > > > > configurations by editing server.properties and
> > > > restarting
> > > > > > the
> > > > > > > > > > broker.
> > > > > > > > > > > > I've
> > > > > > > > > > > > > added the ability to dynamically configure them, and
> > > that
> > > > > > > works well
> > > > > > > > > > > > using
> > > > > > > > > > > > > the AdminClient. However, when I try to update these
> > > > > > > configurations
> > > > > > > > > > using
> > > > > > > > > > > > > kafka-configs.sh, I run into a problem. My
> > > configurations
> > > > > > > contain
> > > > > > > > > > commas,
> > > > > > > > > > > > > and kafka-configs.sh tries to break them up into
> > > > key/value
> > > > > > > pairs at
> > > > > > > > > > the
> > > > > > > > > > > > > comma boundary.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I'd like to enable setting these configurations from
> > > the
> > > > > > > command
> > > > > > > > > > line, so
> > > > > > > > > > > > > I'm proposing that we add a new option to
> > > > kafka-configs.sh
> > > > > > that
> > > > > > > > > > takes a
> > > > > > > > > > > > > properties file.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I've created a KIP for this idea:
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > >
> > > > > >
> > > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > > > > > And a JIRA:
> > > > https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > > > > > >
> > > > > > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > Aneel
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Aneel Nazareth <an...@confluent.io>.
Hi Kamal,
Thanks for taking a look at this KIP.
Unfortunately the user actually can't pass the arguments on the
command line using the existing --add-config option if the values are
complex structures that contain commas. --add-config assumes that
commas separate distinct configuration properties. There's a
workaround using square brackets ("[a,b,c]") for simple lists, but it
doesn't work for things like nested lists or JSON values.
The motivation for allowing STDIN as well as files is to enable
grep/pipe workflows in scripts without creating a temporary file. I
don't know if such workflows will end up being common, and hopefully
someone with a complex enough use case to require it would also be
familiar with techniques for securely creating and cleaning up
temporary files.
I'm okay with excluding the option to allow STDIN in the name of
consistency, if the consensus thinks that's wise. Anyone else have
opinions on this?
On Thu, Mar 26, 2020 at 9:02 AM Kamal Chandraprakash
<ka...@gmail.com> wrote:
>
> Hi Colin,
>
> We should not support STDIN to maintain uniformity across scripts. If the
> user wants to pass the arguments in command line,
> they can always use the existing --add-config option.
>
>
>
>
> On Thu, Mar 26, 2020 at 7:20 PM David Jacot <dj...@confluent.io> wrote:
>
> > Rajini has made a good point. I don't feel strong for either ways but if
> > people
> > are confused by this, it is probably better without it.
> >
> > Best,
> > David
> >
> > On Thu, Mar 26, 2020 at 7:23 AM Colin McCabe <cm...@apache.org> wrote:
> >
> > > Hi Kamal,
> > >
> > > Are you suggesting that we not support STDIN here? I have mixed
> > feelings.
> > >
> > > I think the ideal solution would be to support "-" in these tools
> > whenever
> > > a file argument was expected. But that would be a bigger change than
> > what
> > > we're talking about here. Maybe you are right and we should keep it
> > simple
> > > for now.
> > >
> > > best,
> > > Colin
> > >
> > > On Wed, Mar 25, 2020, at 01:24, Kamal Chandraprakash wrote:
> > > > STDIN wasn't standard practice in other scripts like
> > > > kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> > > > in which the props file is accepted via consumer.config /
> > > producer.config /
> > > > command-config parameter.
> > > >
> > > > Shouldn't we have to maintain the uniformity across scripts?
> > > >
> > > > On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io>
> > wrote:
> > > >
> > > > > Hi Aneel,
> > > > >
> > > > > Thanks for the updated KIP. I have made a second pass over it and the
> > > > > KIP looks good to me.
> > > > >
> > > > > Best,
> > > > > David
> > > > >
> > > > > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io>
> > > wrote:
> > > > >
> > > > > > After reading a bit more about it in the Kubernetes case, I think
> > > it's
> > > > > > reasonable to do this and be explicit that we're ignoring the
> > value,
> > > > > > just deleting all keys that appear in the file.
> > > > > >
> > > > > > I've updated the KIP wiki page to reflect that:
> > > > > >
> > > > > >
> > > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > >
> > > > > > And updated my sample PR:
> > > > > > https://github.com/apache/kafka/pull/8184
> > > > > >
> > > > > > If there are no further comments, I'll request a vote in a few
> > days.
> > > > > >
> > > > > > Thanks for the feedback!
> > > > > >
> > > > > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > > > > wrote:
> > > > > > >
> > > > > > > Hi David,
> > > > > > >
> > > > > > > Is the expected behavior that the keys are deleted without
> > > checking the
> > > > > > values?
> > > > > > >
> > > > > > > Let's say I had this file new.properties:
> > > > > > > a=1
> > > > > > > b=2
> > > > > > >
> > > > > > > And ran:
> > > > > > >
> > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > --entity-type brokers --entity-default \
> > > > > > > --alter --add-config-file new.properties
> > > > > > >
> > > > > > > It seems clear what should happen if I run this immediately:
> > > > > > >
> > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > --entity-type brokers --entity-default \
> > > > > > > --alter --delete-config-file new.properties
> > > > > > >
> > > > > > > (Namely that both a and b would now have no values in the config)
> > > > > > >
> > > > > > > But what if this were run in-between:
> > > > > > >
> > > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > > --entity-type brokers --entity-default \
> > > > > > > --alter --add-config a=3
> > > > > > >
> > > > > > > Would it be surprising if the key/value pair a=3 was deleted,
> > even
> > > > > > > though the config that is in the file is a=1? Or would that be
> > > > > > > expected?
> > > > > > >
> > > > > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > > > > wrote:
> > > > > > > >
> > > > > > > > Hi Colin,
> > > > > > > >
> > > > > > > > Yes, you're right. This is weird but convenient because you
> > don't
> > > > > have
> > > > > > to
> > > > > > > > duplicate
> > > > > > > > the "keys". I was thinking about the kubernetes API which
> > allows
> > > to
> > > > > > create
> > > > > > > > a Pod
> > > > > > > > based on a file and allows to delete it as well with the same
> > > file. I
> > > > > > have
> > > > > > > > always found
> > > > > > > > this convenient, especially when doing local tests.
> > > > > > > >
> > > > > > > > Best,
> > > > > > > > David
> > > > > > > >
> > > > > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <
> > cmccabe@apache.org>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi Aneel,
> > > > > > > > >
> > > > > > > > > Thanks for the KIP. I like the idea.
> > > > > > > > >
> > > > > > > > > You mention that "input from STDIN can be used instead of a
> > > file on
> > > > > > > > > disk." The example given in the KIP seems to suggest that
> > the
> > > > > > command
> > > > > > > > > defaults to reading from STDIN if no argument is given to
> > > > > > --add-config-file.
> > > > > > > > >
> > > > > > > > > I would argue against this particular command-line pattern.
> > > From
> > > > > the
> > > > > > > > > user's point of view, if they mess up and forget to supply an
> > > > > > argument, or
> > > > > > > > > for some reason the parser doesn't treat something like an
> > > > > argument,
> > > > > > the
> > > > > > > > > program will appear to hang in a confusing way.
> > > > > > > > >
> > > > > > > > > Instead, it would be better to follow the traditional UNIX
> > > pattern
> > > > > > where a
> > > > > > > > > dash indicates that STDIN should be read. So
> > > "--add-config-file -"
> > > > > > would
> > > > > > > > > indicate that the program should read form STDIN. This would
> > > be
> > > > > > difficult
> > > > > > > > > to trigger accidentally, and more in line with the
> > traditional
> > > > > > conventions.
> > > > > > > > >
> > > > > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > > > > I wonder if we should also add a `--delete-config-file` as
> > a
> > > > > > counterpart
> > > > > > > > > of
> > > > > > > > > > `--add-config-file`. It would be a bit weird to use a
> > > properties
> > > > > > file in
> > > > > > > > > > this case as the values are not necessary but it may be
> > > handy to
> > > > > > have the
> > > > > > > > > > possibility to remove the configurations which have been
> > set.
> > > > > Have
> > > > > > you
> > > > > > > > > > considered this?
> > > > > > > > >
> > > > > > > > > Hi David,
> > > > > > > > >
> > > > > > > > > That's an interesting idea. However, I think it might be
> > > confusing
> > > > > > to
> > > > > > > > > users to supply a file, and then have the values supplied in
> > > that
> > > > > > file be
> > > > > > > > > ignored. Is there really a case where we need to do this (as
> > > > > > opposed to
> > > > > > > > > creating a file with blank values, or just passing the keys
> > to
> > > > > > > > > --delete-config?
> > > > > > > > >
> > > > > > > > > best,
> > > > > > > > > Colin
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > David
> > > > > > > > > >
> > > > > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > > > > aneel@confluent.io>
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide
> > to
> > > go
> > > > > > ahead
> > > > > > > > > with
> > > > > > > > > > > this KIP.
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > > > > aneel@confluent.io>
> > > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > > Hi,
> > > > > > > > > > > >
> > > > > > > > > > > > I'd like to discuss adding a new argument to
> > > kafka-configs.sh
> > > > > > > > > > > > (ConfigCommand.scala).
> > > > > > > > > > > >
> > > > > > > > > > > > Recently I've been working on some things that require
> > > > > complex
> > > > > > > > > > > > configurations. I've chosen to represent them as JSON
> > > strings
> > > > > > in my
> > > > > > > > > > > > server.properties. This works well, and I'm able to
> > > update
> > > > > the
> > > > > > > > > > > > configurations by editing server.properties and
> > > restarting
> > > > > the
> > > > > > > > > broker.
> > > > > > > > > > > I've
> > > > > > > > > > > > added the ability to dynamically configure them, and
> > that
> > > > > > works well
> > > > > > > > > > > using
> > > > > > > > > > > > the AdminClient. However, when I try to update these
> > > > > > configurations
> > > > > > > > > using
> > > > > > > > > > > > kafka-configs.sh, I run into a problem. My
> > configurations
> > > > > > contain
> > > > > > > > > commas,
> > > > > > > > > > > > and kafka-configs.sh tries to break them up into
> > > key/value
> > > > > > pairs at
> > > > > > > > > the
> > > > > > > > > > > > comma boundary.
> > > > > > > > > > > >
> > > > > > > > > > > > I'd like to enable setting these configurations from
> > the
> > > > > > command
> > > > > > > > > line, so
> > > > > > > > > > > > I'm proposing that we add a new option to
> > > kafka-configs.sh
> > > > > that
> > > > > > > > > takes a
> > > > > > > > > > > > properties file.
> > > > > > > > > > > >
> > > > > > > > > > > > I've created a KIP for this idea:
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > >
> > > > > >
> > > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > > > > And a JIRA:
> > > https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > > > > >
> > > > > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks,
> > > > > > > > > > > > Aneel
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Kamal Chandraprakash <ka...@gmail.com>.
Hi Colin,
We should not support STDIN to maintain uniformity across scripts. If the
user wants to pass the arguments in command line,
they can always use the existing --add-config option.
On Thu, Mar 26, 2020 at 7:20 PM David Jacot <dj...@confluent.io> wrote:
> Rajini has made a good point. I don't feel strong for either ways but if
> people
> are confused by this, it is probably better without it.
>
> Best,
> David
>
> On Thu, Mar 26, 2020 at 7:23 AM Colin McCabe <cm...@apache.org> wrote:
>
> > Hi Kamal,
> >
> > Are you suggesting that we not support STDIN here? I have mixed
> feelings.
> >
> > I think the ideal solution would be to support "-" in these tools
> whenever
> > a file argument was expected. But that would be a bigger change than
> what
> > we're talking about here. Maybe you are right and we should keep it
> simple
> > for now.
> >
> > best,
> > Colin
> >
> > On Wed, Mar 25, 2020, at 01:24, Kamal Chandraprakash wrote:
> > > STDIN wasn't standard practice in other scripts like
> > > kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> > > in which the props file is accepted via consumer.config /
> > producer.config /
> > > command-config parameter.
> > >
> > > Shouldn't we have to maintain the uniformity across scripts?
> > >
> > > On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io>
> wrote:
> > >
> > > > Hi Aneel,
> > > >
> > > > Thanks for the updated KIP. I have made a second pass over it and the
> > > > KIP looks good to me.
> > > >
> > > > Best,
> > > > David
> > > >
> > > > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io>
> > wrote:
> > > >
> > > > > After reading a bit more about it in the Kubernetes case, I think
> > it's
> > > > > reasonable to do this and be explicit that we're ignoring the
> value,
> > > > > just deleting all keys that appear in the file.
> > > > >
> > > > > I've updated the KIP wiki page to reflect that:
> > > > >
> > > > >
> > > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > >
> > > > > And updated my sample PR:
> > > > > https://github.com/apache/kafka/pull/8184
> > > > >
> > > > > If there are no further comments, I'll request a vote in a few
> days.
> > > > >
> > > > > Thanks for the feedback!
> > > > >
> > > > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > > > wrote:
> > > > > >
> > > > > > Hi David,
> > > > > >
> > > > > > Is the expected behavior that the keys are deleted without
> > checking the
> > > > > values?
> > > > > >
> > > > > > Let's say I had this file new.properties:
> > > > > > a=1
> > > > > > b=2
> > > > > >
> > > > > > And ran:
> > > > > >
> > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > --entity-type brokers --entity-default \
> > > > > > --alter --add-config-file new.properties
> > > > > >
> > > > > > It seems clear what should happen if I run this immediately:
> > > > > >
> > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > --entity-type brokers --entity-default \
> > > > > > --alter --delete-config-file new.properties
> > > > > >
> > > > > > (Namely that both a and b would now have no values in the config)
> > > > > >
> > > > > > But what if this were run in-between:
> > > > > >
> > > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > > --entity-type brokers --entity-default \
> > > > > > --alter --add-config a=3
> > > > > >
> > > > > > Would it be surprising if the key/value pair a=3 was deleted,
> even
> > > > > > though the config that is in the file is a=1? Or would that be
> > > > > > expected?
> > > > > >
> > > > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > > > wrote:
> > > > > > >
> > > > > > > Hi Colin,
> > > > > > >
> > > > > > > Yes, you're right. This is weird but convenient because you
> don't
> > > > have
> > > > > to
> > > > > > > duplicate
> > > > > > > the "keys". I was thinking about the kubernetes API which
> allows
> > to
> > > > > create
> > > > > > > a Pod
> > > > > > > based on a file and allows to delete it as well with the same
> > file. I
> > > > > have
> > > > > > > always found
> > > > > > > this convenient, especially when doing local tests.
> > > > > > >
> > > > > > > Best,
> > > > > > > David
> > > > > > >
> > > > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <
> cmccabe@apache.org>
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi Aneel,
> > > > > > > >
> > > > > > > > Thanks for the KIP. I like the idea.
> > > > > > > >
> > > > > > > > You mention that "input from STDIN can be used instead of a
> > file on
> > > > > > > > disk." The example given in the KIP seems to suggest that
> the
> > > > > command
> > > > > > > > defaults to reading from STDIN if no argument is given to
> > > > > --add-config-file.
> > > > > > > >
> > > > > > > > I would argue against this particular command-line pattern.
> > From
> > > > the
> > > > > > > > user's point of view, if they mess up and forget to supply an
> > > > > argument, or
> > > > > > > > for some reason the parser doesn't treat something like an
> > > > argument,
> > > > > the
> > > > > > > > program will appear to hang in a confusing way.
> > > > > > > >
> > > > > > > > Instead, it would be better to follow the traditional UNIX
> > pattern
> > > > > where a
> > > > > > > > dash indicates that STDIN should be read. So
> > "--add-config-file -"
> > > > > would
> > > > > > > > indicate that the program should read form STDIN. This would
> > be
> > > > > difficult
> > > > > > > > to trigger accidentally, and more in line with the
> traditional
> > > > > conventions.
> > > > > > > >
> > > > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > > > I wonder if we should also add a `--delete-config-file` as
> a
> > > > > counterpart
> > > > > > > > of
> > > > > > > > > `--add-config-file`. It would be a bit weird to use a
> > properties
> > > > > file in
> > > > > > > > > this case as the values are not necessary but it may be
> > handy to
> > > > > have the
> > > > > > > > > possibility to remove the configurations which have been
> set.
> > > > Have
> > > > > you
> > > > > > > > > considered this?
> > > > > > > >
> > > > > > > > Hi David,
> > > > > > > >
> > > > > > > > That's an interesting idea. However, I think it might be
> > confusing
> > > > > to
> > > > > > > > users to supply a file, and then have the values supplied in
> > that
> > > > > file be
> > > > > > > > ignored. Is there really a case where we need to do this (as
> > > > > opposed to
> > > > > > > > creating a file with blank values, or just passing the keys
> to
> > > > > > > > --delete-config?
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > > >
> > > > > > > > > David
> > > > > > > > >
> > > > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > > > aneel@confluent.io>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide
> to
> > go
> > > > > ahead
> > > > > > > > with
> > > > > > > > > > this KIP.
> > > > > > > > > >
> > > > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > > > aneel@confluent.io>
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > I'd like to discuss adding a new argument to
> > kafka-configs.sh
> > > > > > > > > > > (ConfigCommand.scala).
> > > > > > > > > > >
> > > > > > > > > > > Recently I've been working on some things that require
> > > > complex
> > > > > > > > > > > configurations. I've chosen to represent them as JSON
> > strings
> > > > > in my
> > > > > > > > > > > server.properties. This works well, and I'm able to
> > update
> > > > the
> > > > > > > > > > > configurations by editing server.properties and
> > restarting
> > > > the
> > > > > > > > broker.
> > > > > > > > > > I've
> > > > > > > > > > > added the ability to dynamically configure them, and
> that
> > > > > works well
> > > > > > > > > > using
> > > > > > > > > > > the AdminClient. However, when I try to update these
> > > > > configurations
> > > > > > > > using
> > > > > > > > > > > kafka-configs.sh, I run into a problem. My
> configurations
> > > > > contain
> > > > > > > > commas,
> > > > > > > > > > > and kafka-configs.sh tries to break them up into
> > key/value
> > > > > pairs at
> > > > > > > > the
> > > > > > > > > > > comma boundary.
> > > > > > > > > > >
> > > > > > > > > > > I'd like to enable setting these configurations from
> the
> > > > > command
> > > > > > > > line, so
> > > > > > > > > > > I'm proposing that we add a new option to
> > kafka-configs.sh
> > > > that
> > > > > > > > takes a
> > > > > > > > > > > properties file.
> > > > > > > > > > >
> > > > > > > > > > > I've created a KIP for this idea:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > >
> > > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > > > And a JIRA:
> > https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > > > >
> > > > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > > > >
> > > > > > > > > > > Thanks,
> > > > > > > > > > > Aneel
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by David Jacot <dj...@confluent.io>.
Rajini has made a good point. I don't feel strong for either ways but if
people
are confused by this, it is probably better without it.
Best,
David
On Thu, Mar 26, 2020 at 7:23 AM Colin McCabe <cm...@apache.org> wrote:
> Hi Kamal,
>
> Are you suggesting that we not support STDIN here? I have mixed feelings.
>
> I think the ideal solution would be to support "-" in these tools whenever
> a file argument was expected. But that would be a bigger change than what
> we're talking about here. Maybe you are right and we should keep it simple
> for now.
>
> best,
> Colin
>
> On Wed, Mar 25, 2020, at 01:24, Kamal Chandraprakash wrote:
> > STDIN wasn't standard practice in other scripts like
> > kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> > in which the props file is accepted via consumer.config /
> producer.config /
> > command-config parameter.
> >
> > Shouldn't we have to maintain the uniformity across scripts?
> >
> > On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io> wrote:
> >
> > > Hi Aneel,
> > >
> > > Thanks for the updated KIP. I have made a second pass over it and the
> > > KIP looks good to me.
> > >
> > > Best,
> > > David
> > >
> > > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io>
> wrote:
> > >
> > > > After reading a bit more about it in the Kubernetes case, I think
> it's
> > > > reasonable to do this and be explicit that we're ignoring the value,
> > > > just deleting all keys that appear in the file.
> > > >
> > > > I've updated the KIP wiki page to reflect that:
> > > >
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > >
> > > > And updated my sample PR:
> > > > https://github.com/apache/kafka/pull/8184
> > > >
> > > > If there are no further comments, I'll request a vote in a few days.
> > > >
> > > > Thanks for the feedback!
> > > >
> > > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > > wrote:
> > > > >
> > > > > Hi David,
> > > > >
> > > > > Is the expected behavior that the keys are deleted without
> checking the
> > > > values?
> > > > >
> > > > > Let's say I had this file new.properties:
> > > > > a=1
> > > > > b=2
> > > > >
> > > > > And ran:
> > > > >
> > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > --entity-type brokers --entity-default \
> > > > > --alter --add-config-file new.properties
> > > > >
> > > > > It seems clear what should happen if I run this immediately:
> > > > >
> > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > --entity-type brokers --entity-default \
> > > > > --alter --delete-config-file new.properties
> > > > >
> > > > > (Namely that both a and b would now have no values in the config)
> > > > >
> > > > > But what if this were run in-between:
> > > > >
> > > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > > --entity-type brokers --entity-default \
> > > > > --alter --add-config a=3
> > > > >
> > > > > Would it be surprising if the key/value pair a=3 was deleted, even
> > > > > though the config that is in the file is a=1? Or would that be
> > > > > expected?
> > > > >
> > > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > > wrote:
> > > > > >
> > > > > > Hi Colin,
> > > > > >
> > > > > > Yes, you're right. This is weird but convenient because you don't
> > > have
> > > > to
> > > > > > duplicate
> > > > > > the "keys". I was thinking about the kubernetes API which allows
> to
> > > > create
> > > > > > a Pod
> > > > > > based on a file and allows to delete it as well with the same
> file. I
> > > > have
> > > > > > always found
> > > > > > this convenient, especially when doing local tests.
> > > > > >
> > > > > > Best,
> > > > > > David
> > > > > >
> > > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <cm...@apache.org>
> > > > wrote:
> > > > > >
> > > > > > > Hi Aneel,
> > > > > > >
> > > > > > > Thanks for the KIP. I like the idea.
> > > > > > >
> > > > > > > You mention that "input from STDIN can be used instead of a
> file on
> > > > > > > disk." The example given in the KIP seems to suggest that the
> > > > command
> > > > > > > defaults to reading from STDIN if no argument is given to
> > > > --add-config-file.
> > > > > > >
> > > > > > > I would argue against this particular command-line pattern.
> From
> > > the
> > > > > > > user's point of view, if they mess up and forget to supply an
> > > > argument, or
> > > > > > > for some reason the parser doesn't treat something like an
> > > argument,
> > > > the
> > > > > > > program will appear to hang in a confusing way.
> > > > > > >
> > > > > > > Instead, it would be better to follow the traditional UNIX
> pattern
> > > > where a
> > > > > > > dash indicates that STDIN should be read. So
> "--add-config-file -"
> > > > would
> > > > > > > indicate that the program should read form STDIN. This would
> be
> > > > difficult
> > > > > > > to trigger accidentally, and more in line with the traditional
> > > > conventions.
> > > > > > >
> > > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > > I wonder if we should also add a `--delete-config-file` as a
> > > > counterpart
> > > > > > > of
> > > > > > > > `--add-config-file`. It would be a bit weird to use a
> properties
> > > > file in
> > > > > > > > this case as the values are not necessary but it may be
> handy to
> > > > have the
> > > > > > > > possibility to remove the configurations which have been set.
> > > Have
> > > > you
> > > > > > > > considered this?
> > > > > > >
> > > > > > > Hi David,
> > > > > > >
> > > > > > > That's an interesting idea. However, I think it might be
> confusing
> > > > to
> > > > > > > users to supply a file, and then have the values supplied in
> that
> > > > file be
> > > > > > > ignored. Is there really a case where we need to do this (as
> > > > opposed to
> > > > > > > creating a file with blank values, or just passing the keys to
> > > > > > > --delete-config?
> > > > > > >
> > > > > > > best,
> > > > > > > Colin
> > > > > > >
> > > > > > > >
> > > > > > > > David
> > > > > > > >
> > > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > > aneel@confluent.io>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide to
> go
> > > > ahead
> > > > > > > with
> > > > > > > > > this KIP.
> > > > > > > > >
> > > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > > aneel@confluent.io>
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > I'd like to discuss adding a new argument to
> kafka-configs.sh
> > > > > > > > > > (ConfigCommand.scala).
> > > > > > > > > >
> > > > > > > > > > Recently I've been working on some things that require
> > > complex
> > > > > > > > > > configurations. I've chosen to represent them as JSON
> strings
> > > > in my
> > > > > > > > > > server.properties. This works well, and I'm able to
> update
> > > the
> > > > > > > > > > configurations by editing server.properties and
> restarting
> > > the
> > > > > > > broker.
> > > > > > > > > I've
> > > > > > > > > > added the ability to dynamically configure them, and that
> > > > works well
> > > > > > > > > using
> > > > > > > > > > the AdminClient. However, when I try to update these
> > > > configurations
> > > > > > > using
> > > > > > > > > > kafka-configs.sh, I run into a problem. My configurations
> > > > contain
> > > > > > > commas,
> > > > > > > > > > and kafka-configs.sh tries to break them up into
> key/value
> > > > pairs at
> > > > > > > the
> > > > > > > > > > comma boundary.
> > > > > > > > > >
> > > > > > > > > > I'd like to enable setting these configurations from the
> > > > command
> > > > > > > line, so
> > > > > > > > > > I'm proposing that we add a new option to
> kafka-configs.sh
> > > that
> > > > > > > takes a
> > > > > > > > > > properties file.
> > > > > > > > > >
> > > > > > > > > > I've created a KIP for this idea:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > > And a JIRA:
> https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > > >
> > > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Aneel
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Colin McCabe <cm...@apache.org>.
Hi Kamal,
Are you suggesting that we not support STDIN here? I have mixed feelings.
I think the ideal solution would be to support "-" in these tools whenever a file argument was expected. But that would be a bigger change than what we're talking about here. Maybe you are right and we should keep it simple for now.
best,
Colin
On Wed, Mar 25, 2020, at 01:24, Kamal Chandraprakash wrote:
> STDIN wasn't standard practice in other scripts like
> kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
> in which the props file is accepted via consumer.config / producer.config /
> command-config parameter.
>
> Shouldn't we have to maintain the uniformity across scripts?
>
> On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io> wrote:
>
> > Hi Aneel,
> >
> > Thanks for the updated KIP. I have made a second pass over it and the
> > KIP looks good to me.
> >
> > Best,
> > David
> >
> > On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io> wrote:
> >
> > > After reading a bit more about it in the Kubernetes case, I think it's
> > > reasonable to do this and be explicit that we're ignoring the value,
> > > just deleting all keys that appear in the file.
> > >
> > > I've updated the KIP wiki page to reflect that:
> > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > >
> > > And updated my sample PR:
> > > https://github.com/apache/kafka/pull/8184
> > >
> > > If there are no further comments, I'll request a vote in a few days.
> > >
> > > Thanks for the feedback!
> > >
> > > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> > wrote:
> > > >
> > > > Hi David,
> > > >
> > > > Is the expected behavior that the keys are deleted without checking the
> > > values?
> > > >
> > > > Let's say I had this file new.properties:
> > > > a=1
> > > > b=2
> > > >
> > > > And ran:
> > > >
> > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > --entity-type brokers --entity-default \
> > > > --alter --add-config-file new.properties
> > > >
> > > > It seems clear what should happen if I run this immediately:
> > > >
> > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > --entity-type brokers --entity-default \
> > > > --alter --delete-config-file new.properties
> > > >
> > > > (Namely that both a and b would now have no values in the config)
> > > >
> > > > But what if this were run in-between:
> > > >
> > > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > > --entity-type brokers --entity-default \
> > > > --alter --add-config a=3
> > > >
> > > > Would it be surprising if the key/value pair a=3 was deleted, even
> > > > though the config that is in the file is a=1? Or would that be
> > > > expected?
> > > >
> > > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> > wrote:
> > > > >
> > > > > Hi Colin,
> > > > >
> > > > > Yes, you're right. This is weird but convenient because you don't
> > have
> > > to
> > > > > duplicate
> > > > > the "keys". I was thinking about the kubernetes API which allows to
> > > create
> > > > > a Pod
> > > > > based on a file and allows to delete it as well with the same file. I
> > > have
> > > > > always found
> > > > > this convenient, especially when doing local tests.
> > > > >
> > > > > Best,
> > > > > David
> > > > >
> > > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <cm...@apache.org>
> > > wrote:
> > > > >
> > > > > > Hi Aneel,
> > > > > >
> > > > > > Thanks for the KIP. I like the idea.
> > > > > >
> > > > > > You mention that "input from STDIN can be used instead of a file on
> > > > > > disk." The example given in the KIP seems to suggest that the
> > > command
> > > > > > defaults to reading from STDIN if no argument is given to
> > > --add-config-file.
> > > > > >
> > > > > > I would argue against this particular command-line pattern. From
> > the
> > > > > > user's point of view, if they mess up and forget to supply an
> > > argument, or
> > > > > > for some reason the parser doesn't treat something like an
> > argument,
> > > the
> > > > > > program will appear to hang in a confusing way.
> > > > > >
> > > > > > Instead, it would be better to follow the traditional UNIX pattern
> > > where a
> > > > > > dash indicates that STDIN should be read. So "--add-config-file -"
> > > would
> > > > > > indicate that the program should read form STDIN. This would be
> > > difficult
> > > > > > to trigger accidentally, and more in line with the traditional
> > > conventions.
> > > > > >
> > > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > > I wonder if we should also add a `--delete-config-file` as a
> > > counterpart
> > > > > > of
> > > > > > > `--add-config-file`. It would be a bit weird to use a properties
> > > file in
> > > > > > > this case as the values are not necessary but it may be handy to
> > > have the
> > > > > > > possibility to remove the configurations which have been set.
> > Have
> > > you
> > > > > > > considered this?
> > > > > >
> > > > > > Hi David,
> > > > > >
> > > > > > That's an interesting idea. However, I think it might be confusing
> > > to
> > > > > > users to supply a file, and then have the values supplied in that
> > > file be
> > > > > > ignored. Is there really a case where we need to do this (as
> > > opposed to
> > > > > > creating a file with blank values, or just passing the keys to
> > > > > > --delete-config?
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > David
> > > > > > >
> > > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > > aneel@confluent.io>
> > > > > > wrote:
> > > > > > >
> > > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > > https://github.com/apache/kafka/pull/8184 if we decide to go
> > > ahead
> > > > > > with
> > > > > > > > this KIP.
> > > > > > > >
> > > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > > aneel@confluent.io>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > I'd like to discuss adding a new argument to kafka-configs.sh
> > > > > > > > > (ConfigCommand.scala).
> > > > > > > > >
> > > > > > > > > Recently I've been working on some things that require
> > complex
> > > > > > > > > configurations. I've chosen to represent them as JSON strings
> > > in my
> > > > > > > > > server.properties. This works well, and I'm able to update
> > the
> > > > > > > > > configurations by editing server.properties and restarting
> > the
> > > > > > broker.
> > > > > > > > I've
> > > > > > > > > added the ability to dynamically configure them, and that
> > > works well
> > > > > > > > using
> > > > > > > > > the AdminClient. However, when I try to update these
> > > configurations
> > > > > > using
> > > > > > > > > kafka-configs.sh, I run into a problem. My configurations
> > > contain
> > > > > > commas,
> > > > > > > > > and kafka-configs.sh tries to break them up into key/value
> > > pairs at
> > > > > > the
> > > > > > > > > comma boundary.
> > > > > > > > >
> > > > > > > > > I'd like to enable setting these configurations from the
> > > command
> > > > > > line, so
> > > > > > > > > I'm proposing that we add a new option to kafka-configs.sh
> > that
> > > > > > takes a
> > > > > > > > > properties file.
> > > > > > > > >
> > > > > > > > > I've created a KIP for this idea:
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > > And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > > >
> > > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Aneel
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > >
> >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Kamal Chandraprakash <ka...@gmail.com>.
STDIN wasn't standard practice in other scripts like
kafka-console-consumer.sh, kafka-console-producer.sh and kafka-acls.sh
in which the props file is accepted via consumer.config / producer.config /
command-config parameter.
Shouldn't we have to maintain the uniformity across scripts?
On Mon, Mar 16, 2020 at 4:13 PM David Jacot <dj...@confluent.io> wrote:
> Hi Aneel,
>
> Thanks for the updated KIP. I have made a second pass over it and the
> KIP looks good to me.
>
> Best,
> David
>
> On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io> wrote:
>
> > After reading a bit more about it in the Kubernetes case, I think it's
> > reasonable to do this and be explicit that we're ignoring the value,
> > just deleting all keys that appear in the file.
> >
> > I've updated the KIP wiki page to reflect that:
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> >
> > And updated my sample PR:
> > https://github.com/apache/kafka/pull/8184
> >
> > If there are no further comments, I'll request a vote in a few days.
> >
> > Thanks for the feedback!
> >
> > On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io>
> wrote:
> > >
> > > Hi David,
> > >
> > > Is the expected behavior that the keys are deleted without checking the
> > values?
> > >
> > > Let's say I had this file new.properties:
> > > a=1
> > > b=2
> > >
> > > And ran:
> > >
> > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > --entity-type brokers --entity-default \
> > > --alter --add-config-file new.properties
> > >
> > > It seems clear what should happen if I run this immediately:
> > >
> > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > --entity-type brokers --entity-default \
> > > --alter --delete-config-file new.properties
> > >
> > > (Namely that both a and b would now have no values in the config)
> > >
> > > But what if this were run in-between:
> > >
> > > bin/kafka-configs --bootstrap-server localhost:9092 \
> > > --entity-type brokers --entity-default \
> > > --alter --add-config a=3
> > >
> > > Would it be surprising if the key/value pair a=3 was deleted, even
> > > though the config that is in the file is a=1? Or would that be
> > > expected?
> > >
> > > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io>
> wrote:
> > > >
> > > > Hi Colin,
> > > >
> > > > Yes, you're right. This is weird but convenient because you don't
> have
> > to
> > > > duplicate
> > > > the "keys". I was thinking about the kubernetes API which allows to
> > create
> > > > a Pod
> > > > based on a file and allows to delete it as well with the same file. I
> > have
> > > > always found
> > > > this convenient, especially when doing local tests.
> > > >
> > > > Best,
> > > > David
> > > >
> > > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <cm...@apache.org>
> > wrote:
> > > >
> > > > > Hi Aneel,
> > > > >
> > > > > Thanks for the KIP. I like the idea.
> > > > >
> > > > > You mention that "input from STDIN can be used instead of a file on
> > > > > disk." The example given in the KIP seems to suggest that the
> > command
> > > > > defaults to reading from STDIN if no argument is given to
> > --add-config-file.
> > > > >
> > > > > I would argue against this particular command-line pattern. From
> the
> > > > > user's point of view, if they mess up and forget to supply an
> > argument, or
> > > > > for some reason the parser doesn't treat something like an
> argument,
> > the
> > > > > program will appear to hang in a confusing way.
> > > > >
> > > > > Instead, it would be better to follow the traditional UNIX pattern
> > where a
> > > > > dash indicates that STDIN should be read. So "--add-config-file -"
> > would
> > > > > indicate that the program should read form STDIN. This would be
> > difficult
> > > > > to trigger accidentally, and more in line with the traditional
> > conventions.
> > > > >
> > > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > > I wonder if we should also add a `--delete-config-file` as a
> > counterpart
> > > > > of
> > > > > > `--add-config-file`. It would be a bit weird to use a properties
> > file in
> > > > > > this case as the values are not necessary but it may be handy to
> > have the
> > > > > > possibility to remove the configurations which have been set.
> Have
> > you
> > > > > > considered this?
> > > > >
> > > > > Hi David,
> > > > >
> > > > > That's an interesting idea. However, I think it might be confusing
> > to
> > > > > users to supply a file, and then have the values supplied in that
> > file be
> > > > > ignored. Is there really a case where we need to do this (as
> > opposed to
> > > > > creating a file with blank values, or just passing the keys to
> > > > > --delete-config?
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > > >
> > > > > > David
> > > > > >
> > > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> > aneel@confluent.io>
> > > > > wrote:
> > > > > >
> > > > > > > I've created a PR for a potential implementation of this:
> > > > > > > https://github.com/apache/kafka/pull/8184 if we decide to go
> > ahead
> > > > > with
> > > > > > > this KIP.
> > > > > > >
> > > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> > aneel@confluent.io>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > I'd like to discuss adding a new argument to kafka-configs.sh
> > > > > > > > (ConfigCommand.scala).
> > > > > > > >
> > > > > > > > Recently I've been working on some things that require
> complex
> > > > > > > > configurations. I've chosen to represent them as JSON strings
> > in my
> > > > > > > > server.properties. This works well, and I'm able to update
> the
> > > > > > > > configurations by editing server.properties and restarting
> the
> > > > > broker.
> > > > > > > I've
> > > > > > > > added the ability to dynamically configure them, and that
> > works well
> > > > > > > using
> > > > > > > > the AdminClient. However, when I try to update these
> > configurations
> > > > > using
> > > > > > > > kafka-configs.sh, I run into a problem. My configurations
> > contain
> > > > > commas,
> > > > > > > > and kafka-configs.sh tries to break them up into key/value
> > pairs at
> > > > > the
> > > > > > > > comma boundary.
> > > > > > > >
> > > > > > > > I'd like to enable setting these configurations from the
> > command
> > > > > line, so
> > > > > > > > I'm proposing that we add a new option to kafka-configs.sh
> that
> > > > > takes a
> > > > > > > > properties file.
> > > > > > > >
> > > > > > > > I've created a KIP for this idea:
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > > And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > > >
> > > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Aneel
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by David Jacot <dj...@confluent.io>.
Hi Aneel,
Thanks for the updated KIP. I have made a second pass over it and the
KIP looks good to me.
Best,
David
On Tue, Mar 10, 2020 at 9:39 PM Aneel Nazareth <an...@confluent.io> wrote:
> After reading a bit more about it in the Kubernetes case, I think it's
> reasonable to do this and be explicit that we're ignoring the value,
> just deleting all keys that appear in the file.
>
> I've updated the KIP wiki page to reflect that:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
>
> And updated my sample PR:
> https://github.com/apache/kafka/pull/8184
>
> If there are no further comments, I'll request a vote in a few days.
>
> Thanks for the feedback!
>
> On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io> wrote:
> >
> > Hi David,
> >
> > Is the expected behavior that the keys are deleted without checking the
> values?
> >
> > Let's say I had this file new.properties:
> > a=1
> > b=2
> >
> > And ran:
> >
> > bin/kafka-configs --bootstrap-server localhost:9092 \
> > --entity-type brokers --entity-default \
> > --alter --add-config-file new.properties
> >
> > It seems clear what should happen if I run this immediately:
> >
> > bin/kafka-configs --bootstrap-server localhost:9092 \
> > --entity-type brokers --entity-default \
> > --alter --delete-config-file new.properties
> >
> > (Namely that both a and b would now have no values in the config)
> >
> > But what if this were run in-between:
> >
> > bin/kafka-configs --bootstrap-server localhost:9092 \
> > --entity-type brokers --entity-default \
> > --alter --add-config a=3
> >
> > Would it be surprising if the key/value pair a=3 was deleted, even
> > though the config that is in the file is a=1? Or would that be
> > expected?
> >
> > On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io> wrote:
> > >
> > > Hi Colin,
> > >
> > > Yes, you're right. This is weird but convenient because you don't have
> to
> > > duplicate
> > > the "keys". I was thinking about the kubernetes API which allows to
> create
> > > a Pod
> > > based on a file and allows to delete it as well with the same file. I
> have
> > > always found
> > > this convenient, especially when doing local tests.
> > >
> > > Best,
> > > David
> > >
> > > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <cm...@apache.org>
> wrote:
> > >
> > > > Hi Aneel,
> > > >
> > > > Thanks for the KIP. I like the idea.
> > > >
> > > > You mention that "input from STDIN can be used instead of a file on
> > > > disk." The example given in the KIP seems to suggest that the
> command
> > > > defaults to reading from STDIN if no argument is given to
> --add-config-file.
> > > >
> > > > I would argue against this particular command-line pattern. From the
> > > > user's point of view, if they mess up and forget to supply an
> argument, or
> > > > for some reason the parser doesn't treat something like an argument,
> the
> > > > program will appear to hang in a confusing way.
> > > >
> > > > Instead, it would be better to follow the traditional UNIX pattern
> where a
> > > > dash indicates that STDIN should be read. So "--add-config-file -"
> would
> > > > indicate that the program should read form STDIN. This would be
> difficult
> > > > to trigger accidentally, and more in line with the traditional
> conventions.
> > > >
> > > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > > I wonder if we should also add a `--delete-config-file` as a
> counterpart
> > > > of
> > > > > `--add-config-file`. It would be a bit weird to use a properties
> file in
> > > > > this case as the values are not necessary but it may be handy to
> have the
> > > > > possibility to remove the configurations which have been set. Have
> you
> > > > > considered this?
> > > >
> > > > Hi David,
> > > >
> > > > That's an interesting idea. However, I think it might be confusing
> to
> > > > users to supply a file, and then have the values supplied in that
> file be
> > > > ignored. Is there really a case where we need to do this (as
> opposed to
> > > > creating a file with blank values, or just passing the keys to
> > > > --delete-config?
> > > >
> > > > best,
> > > > Colin
> > > >
> > > > >
> > > > > David
> > > > >
> > > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <
> aneel@confluent.io>
> > > > wrote:
> > > > >
> > > > > > I've created a PR for a potential implementation of this:
> > > > > > https://github.com/apache/kafka/pull/8184 if we decide to go
> ahead
> > > > with
> > > > > > this KIP.
> > > > > >
> > > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <
> aneel@confluent.io>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > I'd like to discuss adding a new argument to kafka-configs.sh
> > > > > > > (ConfigCommand.scala).
> > > > > > >
> > > > > > > Recently I've been working on some things that require complex
> > > > > > > configurations. I've chosen to represent them as JSON strings
> in my
> > > > > > > server.properties. This works well, and I'm able to update the
> > > > > > > configurations by editing server.properties and restarting the
> > > > broker.
> > > > > > I've
> > > > > > > added the ability to dynamically configure them, and that
> works well
> > > > > > using
> > > > > > > the AdminClient. However, when I try to update these
> configurations
> > > > using
> > > > > > > kafka-configs.sh, I run into a problem. My configurations
> contain
> > > > commas,
> > > > > > > and kafka-configs.sh tries to break them up into key/value
> pairs at
> > > > the
> > > > > > > comma boundary.
> > > > > > >
> > > > > > > I'd like to enable setting these configurations from the
> command
> > > > line, so
> > > > > > > I'm proposing that we add a new option to kafka-configs.sh that
> > > > takes a
> > > > > > > properties file.
> > > > > > >
> > > > > > > I've created a KIP for this idea:
> > > > > > >
> > > > > > >
> > > > > >
> > > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > > And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > > >
> > > > > > > I'd appreciate your feedback on the proposal.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Aneel
> > > > > > >
> > > > > >
> > > > >
> > > >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Aneel Nazareth <an...@confluent.io>.
After reading a bit more about it in the Kubernetes case, I think it's
reasonable to do this and be explicit that we're ignoring the value,
just deleting all keys that appear in the file.
I've updated the KIP wiki page to reflect that:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
And updated my sample PR:
https://github.com/apache/kafka/pull/8184
If there are no further comments, I'll request a vote in a few days.
Thanks for the feedback!
On Mon, Mar 9, 2020 at 1:24 PM Aneel Nazareth <an...@confluent.io> wrote:
>
> Hi David,
>
> Is the expected behavior that the keys are deleted without checking the values?
>
> Let's say I had this file new.properties:
> a=1
> b=2
>
> And ran:
>
> bin/kafka-configs --bootstrap-server localhost:9092 \
> --entity-type brokers --entity-default \
> --alter --add-config-file new.properties
>
> It seems clear what should happen if I run this immediately:
>
> bin/kafka-configs --bootstrap-server localhost:9092 \
> --entity-type brokers --entity-default \
> --alter --delete-config-file new.properties
>
> (Namely that both a and b would now have no values in the config)
>
> But what if this were run in-between:
>
> bin/kafka-configs --bootstrap-server localhost:9092 \
> --entity-type brokers --entity-default \
> --alter --add-config a=3
>
> Would it be surprising if the key/value pair a=3 was deleted, even
> though the config that is in the file is a=1? Or would that be
> expected?
>
> On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io> wrote:
> >
> > Hi Colin,
> >
> > Yes, you're right. This is weird but convenient because you don't have to
> > duplicate
> > the "keys". I was thinking about the kubernetes API which allows to create
> > a Pod
> > based on a file and allows to delete it as well with the same file. I have
> > always found
> > this convenient, especially when doing local tests.
> >
> > Best,
> > David
> >
> > On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <cm...@apache.org> wrote:
> >
> > > Hi Aneel,
> > >
> > > Thanks for the KIP. I like the idea.
> > >
> > > You mention that "input from STDIN can be used instead of a file on
> > > disk." The example given in the KIP seems to suggest that the command
> > > defaults to reading from STDIN if no argument is given to --add-config-file.
> > >
> > > I would argue against this particular command-line pattern. From the
> > > user's point of view, if they mess up and forget to supply an argument, or
> > > for some reason the parser doesn't treat something like an argument, the
> > > program will appear to hang in a confusing way.
> > >
> > > Instead, it would be better to follow the traditional UNIX pattern where a
> > > dash indicates that STDIN should be read. So "--add-config-file -" would
> > > indicate that the program should read form STDIN. This would be difficult
> > > to trigger accidentally, and more in line with the traditional conventions.
> > >
> > > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > > I wonder if we should also add a `--delete-config-file` as a counterpart
> > > of
> > > > `--add-config-file`. It would be a bit weird to use a properties file in
> > > > this case as the values are not necessary but it may be handy to have the
> > > > possibility to remove the configurations which have been set. Have you
> > > > considered this?
> > >
> > > Hi David,
> > >
> > > That's an interesting idea. However, I think it might be confusing to
> > > users to supply a file, and then have the values supplied in that file be
> > > ignored. Is there really a case where we need to do this (as opposed to
> > > creating a file with blank values, or just passing the keys to
> > > --delete-config?
> > >
> > > best,
> > > Colin
> > >
> > > >
> > > > David
> > > >
> > > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <an...@confluent.io>
> > > wrote:
> > > >
> > > > > I've created a PR for a potential implementation of this:
> > > > > https://github.com/apache/kafka/pull/8184 if we decide to go ahead
> > > with
> > > > > this KIP.
> > > > >
> > > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <an...@confluent.io>
> > > > > wrote:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > I'd like to discuss adding a new argument to kafka-configs.sh
> > > > > > (ConfigCommand.scala).
> > > > > >
> > > > > > Recently I've been working on some things that require complex
> > > > > > configurations. I've chosen to represent them as JSON strings in my
> > > > > > server.properties. This works well, and I'm able to update the
> > > > > > configurations by editing server.properties and restarting the
> > > broker.
> > > > > I've
> > > > > > added the ability to dynamically configure them, and that works well
> > > > > using
> > > > > > the AdminClient. However, when I try to update these configurations
> > > using
> > > > > > kafka-configs.sh, I run into a problem. My configurations contain
> > > commas,
> > > > > > and kafka-configs.sh tries to break them up into key/value pairs at
> > > the
> > > > > > comma boundary.
> > > > > >
> > > > > > I'd like to enable setting these configurations from the command
> > > line, so
> > > > > > I'm proposing that we add a new option to kafka-configs.sh that
> > > takes a
> > > > > > properties file.
> > > > > >
> > > > > > I've created a KIP for this idea:
> > > > > >
> > > > > >
> > > > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > > And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
> > > > > >
> > > > > > I'd appreciate your feedback on the proposal.
> > > > > >
> > > > > > Thanks,
> > > > > > Aneel
> > > > > >
> > > > >
> > > >
> > >
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Aneel Nazareth <an...@confluent.io>.
Hi David,
Is the expected behavior that the keys are deleted without checking the values?
Let's say I had this file new.properties:
a=1
b=2
And ran:
bin/kafka-configs --bootstrap-server localhost:9092 \
--entity-type brokers --entity-default \
--alter --add-config-file new.properties
It seems clear what should happen if I run this immediately:
bin/kafka-configs --bootstrap-server localhost:9092 \
--entity-type brokers --entity-default \
--alter --delete-config-file new.properties
(Namely that both a and b would now have no values in the config)
But what if this were run in-between:
bin/kafka-configs --bootstrap-server localhost:9092 \
--entity-type brokers --entity-default \
--alter --add-config a=3
Would it be surprising if the key/value pair a=3 was deleted, even
though the config that is in the file is a=1? Or would that be
expected?
On Mon, Mar 9, 2020 at 1:02 PM David Jacot <dj...@confluent.io> wrote:
>
> Hi Colin,
>
> Yes, you're right. This is weird but convenient because you don't have to
> duplicate
> the "keys". I was thinking about the kubernetes API which allows to create
> a Pod
> based on a file and allows to delete it as well with the same file. I have
> always found
> this convenient, especially when doing local tests.
>
> Best,
> David
>
> On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <cm...@apache.org> wrote:
>
> > Hi Aneel,
> >
> > Thanks for the KIP. I like the idea.
> >
> > You mention that "input from STDIN can be used instead of a file on
> > disk." The example given in the KIP seems to suggest that the command
> > defaults to reading from STDIN if no argument is given to --add-config-file.
> >
> > I would argue against this particular command-line pattern. From the
> > user's point of view, if they mess up and forget to supply an argument, or
> > for some reason the parser doesn't treat something like an argument, the
> > program will appear to hang in a confusing way.
> >
> > Instead, it would be better to follow the traditional UNIX pattern where a
> > dash indicates that STDIN should be read. So "--add-config-file -" would
> > indicate that the program should read form STDIN. This would be difficult
> > to trigger accidentally, and more in line with the traditional conventions.
> >
> > On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > > I wonder if we should also add a `--delete-config-file` as a counterpart
> > of
> > > `--add-config-file`. It would be a bit weird to use a properties file in
> > > this case as the values are not necessary but it may be handy to have the
> > > possibility to remove the configurations which have been set. Have you
> > > considered this?
> >
> > Hi David,
> >
> > That's an interesting idea. However, I think it might be confusing to
> > users to supply a file, and then have the values supplied in that file be
> > ignored. Is there really a case where we need to do this (as opposed to
> > creating a file with blank values, or just passing the keys to
> > --delete-config?
> >
> > best,
> > Colin
> >
> > >
> > > David
> > >
> > > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <an...@confluent.io>
> > wrote:
> > >
> > > > I've created a PR for a potential implementation of this:
> > > > https://github.com/apache/kafka/pull/8184 if we decide to go ahead
> > with
> > > > this KIP.
> > > >
> > > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <an...@confluent.io>
> > > > wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > I'd like to discuss adding a new argument to kafka-configs.sh
> > > > > (ConfigCommand.scala).
> > > > >
> > > > > Recently I've been working on some things that require complex
> > > > > configurations. I've chosen to represent them as JSON strings in my
> > > > > server.properties. This works well, and I'm able to update the
> > > > > configurations by editing server.properties and restarting the
> > broker.
> > > > I've
> > > > > added the ability to dynamically configure them, and that works well
> > > > using
> > > > > the AdminClient. However, when I try to update these configurations
> > using
> > > > > kafka-configs.sh, I run into a problem. My configurations contain
> > commas,
> > > > > and kafka-configs.sh tries to break them up into key/value pairs at
> > the
> > > > > comma boundary.
> > > > >
> > > > > I'd like to enable setting these configurations from the command
> > line, so
> > > > > I'm proposing that we add a new option to kafka-configs.sh that
> > takes a
> > > > > properties file.
> > > > >
> > > > > I've created a KIP for this idea:
> > > > >
> > > > >
> > > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > > And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
> > > > >
> > > > > I'd appreciate your feedback on the proposal.
> > > > >
> > > > > Thanks,
> > > > > Aneel
> > > > >
> > > >
> > >
> >
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by David Jacot <dj...@confluent.io>.
Hi Colin,
Yes, you're right. This is weird but convenient because you don't have to
duplicate
the "keys". I was thinking about the kubernetes API which allows to create
a Pod
based on a file and allows to delete it as well with the same file. I have
always found
this convenient, especially when doing local tests.
Best,
David
On Mon, Mar 9, 2020 at 6:35 PM Colin McCabe <cm...@apache.org> wrote:
> Hi Aneel,
>
> Thanks for the KIP. I like the idea.
>
> You mention that "input from STDIN can be used instead of a file on
> disk." The example given in the KIP seems to suggest that the command
> defaults to reading from STDIN if no argument is given to --add-config-file.
>
> I would argue against this particular command-line pattern. From the
> user's point of view, if they mess up and forget to supply an argument, or
> for some reason the parser doesn't treat something like an argument, the
> program will appear to hang in a confusing way.
>
> Instead, it would be better to follow the traditional UNIX pattern where a
> dash indicates that STDIN should be read. So "--add-config-file -" would
> indicate that the program should read form STDIN. This would be difficult
> to trigger accidentally, and more in line with the traditional conventions.
>
> On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> > I wonder if we should also add a `--delete-config-file` as a counterpart
> of
> > `--add-config-file`. It would be a bit weird to use a properties file in
> > this case as the values are not necessary but it may be handy to have the
> > possibility to remove the configurations which have been set. Have you
> > considered this?
>
> Hi David,
>
> That's an interesting idea. However, I think it might be confusing to
> users to supply a file, and then have the values supplied in that file be
> ignored. Is there really a case where we need to do this (as opposed to
> creating a file with blank values, or just passing the keys to
> --delete-config?
>
> best,
> Colin
>
> >
> > David
> >
> > On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <an...@confluent.io>
> wrote:
> >
> > > I've created a PR for a potential implementation of this:
> > > https://github.com/apache/kafka/pull/8184 if we decide to go ahead
> with
> > > this KIP.
> > >
> > > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <an...@confluent.io>
> > > wrote:
> > >
> > > > Hi,
> > > >
> > > > I'd like to discuss adding a new argument to kafka-configs.sh
> > > > (ConfigCommand.scala).
> > > >
> > > > Recently I've been working on some things that require complex
> > > > configurations. I've chosen to represent them as JSON strings in my
> > > > server.properties. This works well, and I'm able to update the
> > > > configurations by editing server.properties and restarting the
> broker.
> > > I've
> > > > added the ability to dynamically configure them, and that works well
> > > using
> > > > the AdminClient. However, when I try to update these configurations
> using
> > > > kafka-configs.sh, I run into a problem. My configurations contain
> commas,
> > > > and kafka-configs.sh tries to break them up into key/value pairs at
> the
> > > > comma boundary.
> > > >
> > > > I'd like to enable setting these configurations from the command
> line, so
> > > > I'm proposing that we add a new option to kafka-configs.sh that
> takes a
> > > > properties file.
> > > >
> > > > I've created a KIP for this idea:
> > > >
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > > And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
> > > >
> > > > I'd appreciate your feedback on the proposal.
> > > >
> > > > Thanks,
> > > > Aneel
> > > >
> > >
> >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Colin McCabe <cm...@apache.org>.
Hi Aneel,
Thanks for the KIP. I like the idea.
You mention that "input from STDIN can be used instead of a file on disk." The example given in the KIP seems to suggest that the command defaults to reading from STDIN if no argument is given to --add-config-file.
I would argue against this particular command-line pattern. From the user's point of view, if they mess up and forget to supply an argument, or for some reason the parser doesn't treat something like an argument, the program will appear to hang in a confusing way.
Instead, it would be better to follow the traditional UNIX pattern where a dash indicates that STDIN should be read. So "--add-config-file -" would indicate that the program should read form STDIN. This would be difficult to trigger accidentally, and more in line with the traditional conventions.
On Mon, Mar 9, 2020, at 08:47, David Jacot wrote:
> I wonder if we should also add a `--delete-config-file` as a counterpart of
> `--add-config-file`. It would be a bit weird to use a properties file in
> this case as the values are not necessary but it may be handy to have the
> possibility to remove the configurations which have been set. Have you
> considered this?
Hi David,
That's an interesting idea. However, I think it might be confusing to users to supply a file, and then have the values supplied in that file be ignored. Is there really a case where we need to do this (as opposed to creating a file with blank values, or just passing the keys to --delete-config?
best,
Colin
>
> David
>
> On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <an...@confluent.io> wrote:
>
> > I've created a PR for a potential implementation of this:
> > https://github.com/apache/kafka/pull/8184 if we decide to go ahead with
> > this KIP.
> >
> > On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <an...@confluent.io>
> > wrote:
> >
> > > Hi,
> > >
> > > I'd like to discuss adding a new argument to kafka-configs.sh
> > > (ConfigCommand.scala).
> > >
> > > Recently I've been working on some things that require complex
> > > configurations. I've chosen to represent them as JSON strings in my
> > > server.properties. This works well, and I'm able to update the
> > > configurations by editing server.properties and restarting the broker.
> > I've
> > > added the ability to dynamically configure them, and that works well
> > using
> > > the AdminClient. However, when I try to update these configurations using
> > > kafka-configs.sh, I run into a problem. My configurations contain commas,
> > > and kafka-configs.sh tries to break them up into key/value pairs at the
> > > comma boundary.
> > >
> > > I'd like to enable setting these configurations from the command line, so
> > > I'm proposing that we add a new option to kafka-configs.sh that takes a
> > > properties file.
> > >
> > > I've created a KIP for this idea:
> > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > > And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
> > >
> > > I'd appreciate your feedback on the proposal.
> > >
> > > Thanks,
> > > Aneel
> > >
> >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by David Jacot <dj...@confluent.io>.
Hi Aneel,
Thank you for the KIP. I agree that managing complex configurations is not
easy
with the current tool. Having the possibility to use properties file sounds
quite
handy to me. It makes it easier to edit and to reuse base configurations.
I wonder if we should also add a `--delete-config-file` as a counterpart of
`--add-config-file`. It would be a bit weird to use a properties file in
this
case as the values are not necessary but it may be handy to have the
possibility to remove the configurations which have been set. Have you
considered this?
David
On Thu, Feb 27, 2020 at 11:15 PM Aneel Nazareth <an...@confluent.io> wrote:
> I've created a PR for a potential implementation of this:
> https://github.com/apache/kafka/pull/8184 if we decide to go ahead with
> this KIP.
>
> On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <an...@confluent.io>
> wrote:
>
> > Hi,
> >
> > I'd like to discuss adding a new argument to kafka-configs.sh
> > (ConfigCommand.scala).
> >
> > Recently I've been working on some things that require complex
> > configurations. I've chosen to represent them as JSON strings in my
> > server.properties. This works well, and I'm able to update the
> > configurations by editing server.properties and restarting the broker.
> I've
> > added the ability to dynamically configure them, and that works well
> using
> > the AdminClient. However, when I try to update these configurations using
> > kafka-configs.sh, I run into a problem. My configurations contain commas,
> > and kafka-configs.sh tries to break them up into key/value pairs at the
> > comma boundary.
> >
> > I'd like to enable setting these configurations from the command line, so
> > I'm proposing that we add a new option to kafka-configs.sh that takes a
> > properties file.
> >
> > I've created a KIP for this idea:
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> > And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
> >
> > I'd appreciate your feedback on the proposal.
> >
> > Thanks,
> > Aneel
> >
>
Re: [DISCUSS] KIP-574: CLI Dynamic Configuration with file input
Posted by Aneel Nazareth <an...@confluent.io>.
I've created a PR for a potential implementation of this:
https://github.com/apache/kafka/pull/8184 if we decide to go ahead with
this KIP.
On Wed, Feb 26, 2020 at 12:36 PM Aneel Nazareth <an...@confluent.io> wrote:
> Hi,
>
> I'd like to discuss adding a new argument to kafka-configs.sh
> (ConfigCommand.scala).
>
> Recently I've been working on some things that require complex
> configurations. I've chosen to represent them as JSON strings in my
> server.properties. This works well, and I'm able to update the
> configurations by editing server.properties and restarting the broker. I've
> added the ability to dynamically configure them, and that works well using
> the AdminClient. However, when I try to update these configurations using
> kafka-configs.sh, I run into a problem. My configurations contain commas,
> and kafka-configs.sh tries to break them up into key/value pairs at the
> comma boundary.
>
> I'd like to enable setting these configurations from the command line, so
> I'm proposing that we add a new option to kafka-configs.sh that takes a
> properties file.
>
> I've created a KIP for this idea:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-574%3A+CLI+Dynamic+Configuration+with+file+input
> And a JIRA: https://issues.apache.org/jira/browse/KAFKA-9612
>
> I'd appreciate your feedback on the proposal.
>
> Thanks,
> Aneel
>