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
<kamal.chandraprak...@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 <dja...@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 <cmcc...@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 <dja...@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 <dja...@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 <
> > cmcc...@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
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
