> On Nov. 7, 2013, 6:29 p.m., joel koshy wrote: > > core/src/main/scala/kafka/admin/AdminUtils.scala, line 224 > > <https://reviews.apache.org/r/15274/diff/4/?file=380481#file380481line224> > > > > Not sure if this is required any more if we're doing validation earlier > > - since a validation failure would be fatal.
The reason I kept it in the internal API (changeTopicConfigs) is if we use it in places other than tools, it is better to double check the values written to Zookeeper instead of ending up with corrupted topic configs > On Nov. 7, 2013, 6:29 p.m., joel koshy wrote: > > core/src/main/scala/kafka/admin/TopicCommand.scala, line 164 > > <https://reviews.apache.org/r/15274/diff/4/?file=380482#file380482line164> > > > > This shouldn't be called for configsToBeDeleted right? - i.e., > > validation could fail for deleted configs which have no associated value. Refactored this in the latest patch and simplified the logic. Could you take another look please? - Neha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28393 ----------------------------------------------------------- On Nov. 8, 2013, 1:07 a.m., Neha Narkhede wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15274/ > ----------------------------------------------------------- > > (Updated Nov. 8, 2013, 1:07 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1119 > https://issues.apache.org/jira/browse/KAFKA-1119 > > > Repository: kafka > > > Description > ------- > > Improved the patch to simplify the usage of the internal changeTopicConfigs > API. It now includes only the final list of configs to be applied to a topic. > Any additions/deletions should be done prior to invoking this API > > > 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 > >
