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