> On Nov. 7, 2013, 5:57 p.m., joel koshy wrote: > > core/src/main/scala/kafka/admin/TopicCommand.scala, line 153 > > <https://reviews.apache.org/r/15274/diff/2/?file=379572#file379572line153> > > > > This convention should be fine, but personally would prefer an unconfig > > or del-config since people can easily make mistakes and miss the actual > > config value they intend to alter.
Introduced --deleteConfig in the latest patch. > On Nov. 7, 2013, 5:57 p.m., joel koshy wrote: > > core/src/main/scala/kafka/admin/TopicCommand.scala, line 151 > > <https://reviews.apache.org/r/15274/diff/2/?file=379572#file379572line151> > > > > As part of this patch, can you change the regex to use the """ format: > > > > i.e., """\s*=\s*""" > > > > I think that's clearer in that it avoids double escapes. Also, we > > should probably trim the split strings. Included the escapes change. Regarding the trim, included it but probably not required since the tool errors out for - nnarkhed-mn:kafka-git-idea nnarkhed$ ./bin/kafka-topics.sh --zookeeper localhost:2181 --topic shutdown-test3 --alter --config "segment.ms = 180000" Exception in thread "main" java.lang.IllegalArgumentException: requirement failed: Invalid topic config: all configs to be added must be in the format "key=val". at scala.Predef$.require(Predef.scala:145) at kafka.admin.TopicCommand$.parseTopicConfigs(TopicCommand.scala:156) at kafka.admin.TopicCommand$.alterTopic(TopicCommand.scala:84) at kafka.admin.TopicCommand$.main(TopicCommand.scala:51) at kafka.admin.TopicCommand.main(TopicCommand.scala) Works for - nnarkhed-mn:kafka-git-idea nnarkhed$ ./bin/kafka-topics.sh --zookeeper localhost:2181 --topic shutdown-test3 --alter --config "retention.ms=180000" Updated config for topic "shutdown-test3". nnarkhed-mn:kafka-git-idea nnarkhed$ ./bin/kafka-topics.sh --zookeeper localhost:2181 --topic shutdown-test3 --describe shutdown-test3 configs: retention.ms = 180000 partitions: 2 topic: shutdown-test3 partition: 0 leader: 0 replicas: 0 isr: 0 topic: shutdown-test3 partition: 1 leader: 0 replicas: 0 isr: 0 > On Nov. 7, 2013, 5:57 p.m., joel koshy wrote: > > core/src/main/scala/kafka/admin/AdminUtils.scala, line 225 > > <https://reviews.apache.org/r/15274/diff/2/?file=379571#file379571line225> > > > > Would it be better to do validation of all configs (to be removed and > > altered) earlier - i.e., in parseTopicConfigs? > > Added it to parseTopicConfigs, but left it in changeTopicConfigs since that is where the zookeeper write happens. - Neha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28385 ----------------------------------------------------------- On Nov. 7, 2013, 6:17 p.m., Neha Narkhede wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15274/ > ----------------------------------------------------------- > > (Updated Nov. 7, 2013, 6:17 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1119 > https://issues.apache.org/jira/browse/KAFKA-1119 > > > Repository: kafka > > > Description > ------- > > Addressed Joel's review comments > > > Add an explicit --deleteConfig option > > > more cleanup > > > Code cleanup > > > Fixed bug that now allows removing all the topic overrides correctly and > falling back to the defaults > > > 1. Change the --config to diff the previous configs 2. Add the ability to > remove per topic overrides if config is specified without a value > > > Diffs > ----- > > core/src/main/scala/kafka/admin/AdminUtils.scala > 8107a64cf1ef1cac763e152bae9f835411c9d3f3 > core/src/main/scala/kafka/admin/TopicCommand.scala > 56f3177e28a34df0ace1d192aef0060cb5e235df > core/src/main/scala/kafka/log/LogConfig.scala > 51ec796e9e6a10b76daefbd9aea02121fc1d573a > core/src/main/scala/kafka/server/TopicConfigManager.scala > 56cae58f2a216dcba88b2656fd4a490f11461270 > > Diff: https://reviews.apache.org/r/15274/diff/ > > > Testing > ------- > > Locally tested - 1. Adding new config 2. Adding new invalid config 3. > Deleting config 4. Deleting all config overrides > > > Thanks, > > Neha Narkhede > >
