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.chandraprak...@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 <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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
