Re: Review Request 15274: Patch for KAFKA-1119
On Nov. 6, 2013, 9:41 p.m., Guozhang Wang wrote: Topic config change as well as create-topic, add-partition, partition-reassignment and preferred leader election are all asynchronous in the sense that the admin command would return immediately and one has to check himself if the process has finished; I think it is better to add an option to make these commands blocking until the process is done. Guozhang, that is a good suggestion and applies to all tools. Would you mind filing a JIRA for that change? - Neha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28319 --- On Nov. 6, 2013, 6:13 p.m., Neha Narkhede wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/ --- (Updated Nov. 6, 2013, 6:13 p.m.) Review request for kafka. Bugs: KAFKA-1119 https://issues.apache.org/jira/browse/KAFKA-1119 Repository: kafka Description --- 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 --- Thanks, Neha Narkhede
Re: Review Request 15274: Patch for KAFKA-1119
On Nov. 6, 2013, 10:42 p.m., Swapnil Ghike wrote: It will help to add a --deleteConfig option. We can use it as --config config1=newVal --deleteConfig config2. Neha Narkhede wrote: Like we discussed offline, deleteConfig complicates the ability to accept configs easily in bulk and apply those. Actually, I think I misunderstood your suggestion earlier. This makes sense - Neha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28327 --- On Nov. 7, 2013, 4:50 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, 4:50 p.m.) Review request for kafka. Bugs: KAFKA-1119 https://issues.apache.org/jira/browse/KAFKA-1119 Repository: kafka Description --- 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
Re: Review Request 15274: Patch for KAFKA-1119
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/ --- (Updated Nov. 7, 2013, 5:50 p.m.) Review request for kafka. Bugs: KAFKA-1119 https://issues.apache.org/jira/browse/KAFKA-1119 Repository: kafka Description (updated) --- 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 (updated) - 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
Re: Review Request 15274: Patch for KAFKA-1119
On Nov. 6, 2013, 9:41 p.m., Guozhang Wang wrote: Topic config change as well as create-topic, add-partition, partition-reassignment and preferred leader election are all asynchronous in the sense that the admin command would return immediately and one has to check himself if the process has finished; I think it is better to add an option to make these commands blocking until the process is done. Neha Narkhede wrote: Guozhang, that is a good suggestion and applies to all tools. Would you mind filing a JIRA for that change? Done. - Guozhang --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28319 --- On Nov. 7, 2013, 5:50 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, 5:50 p.m.) Review request for kafka. Bugs: KAFKA-1119 https://issues.apache.org/jira/browse/KAFKA-1119 Repository: kafka Description --- 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
Re: Review Request 15274: Patch for KAFKA-1119
--- 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 (updated) --- 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 (updated) - 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
Re: Review Request 15274: Patch for KAFKA-1119
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 = 18 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=18 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 = 18 partitions: 2 topic: shutdown-test3 partition: 0leader: 0 replicas: 0 isr: 0 topic: shutdown-test3 partition: 1leader: 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
Re: Review Request 15274: Patch for KAFKA-1119
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28393 --- core/src/main/scala/kafka/admin/AdminUtils.scala https://reviews.apache.org/r/15274/#comment55224 Not sure if this is required any more if we're doing validation earlier - since a validation failure would be fatal. core/src/main/scala/kafka/admin/TopicCommand.scala https://reviews.apache.org/r/15274/#comment55223 This shouldn't be called for configsToBeDeleted right? - i.e., validation could fail for deleted configs which have no associated value. - joel koshy 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
Re: Review Request 15274: Patch for KAFKA-1119
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28395 --- core/src/main/scala/kafka/admin/TopicCommand.scala https://reviews.apache.org/r/15274/#comment55226 Also, if we are adding the deleteConfig option we need not use the same regex and split. i.e., we expect only one argument. - joel koshy 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
Re: Review Request 15274: Patch for KAFKA-1119
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28440 --- core/src/main/scala/kafka/admin/TopicCommand.scala https://reviews.apache.org/r/15274/#comment55256 Should we enforce that configsToBeAdded and configsToBeDeleted should not contain the same config? - Swapnil Ghike 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
Re: Review Request 15274: Patch for KAFKA-1119
--- 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 (updated) --- 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 (updated) - 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
Re: Review Request 15274: Patch for KAFKA-1119
On Nov. 7, 2013, 6:31 p.m., joel koshy wrote: core/src/main/scala/kafka/admin/TopicCommand.scala, line 153 https://reviews.apache.org/r/15274/diff/4/?file=380482#file380482line153 Also, if we are adding the deleteConfig option we need not use the same regex and split. i.e., we expect only one argument. That's true but the most common mistake while using this tool could be to use deleteConfig like config. So I included the extra check in order to give a relevant error message. - Neha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28395 --- 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
Re: Review Request 15274: Patch for KAFKA-1119
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
Re: Review Request 15274: Patch for KAFKA-1119
On Nov. 7, 2013, 6:31 p.m., joel koshy wrote: core/src/main/scala/kafka/admin/TopicCommand.scala, line 153 https://reviews.apache.org/r/15274/diff/4/?file=380482#file380482line153 Also, if we are adding the deleteConfig option we need not use the same regex and split. i.e., we expect only one argument. Neha Narkhede wrote: That's true but the most common mistake while using this tool could be to use deleteConfig like config. So I included the extra check in order to give a relevant error message. Ok - although I think the config validation would catch that if it is used like config (since there would be whitespace in the option value). - joel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28395 --- 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
Re: Review Request 15274: Patch for KAFKA-1119
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. Neha Narkhede wrote: 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 Good point - makes sense. - joel --- 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
Re: Review Request 15274: Patch for KAFKA-1119
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28481 --- Ship it! - joel koshy 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
Re: Review Request 15274: Patch for KAFKA-1119
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/ --- (Updated Nov. 6, 2013, 6:13 p.m.) Review request for kafka. Bugs: KAFKA-1119 https://issues.apache.org/jira/browse/KAFKA-1119 Repository: kafka Description (updated) --- 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 (updated) - 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 --- Thanks, Neha Narkhede
Re: Review Request 15274: Patch for KAFKA-1119
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28283 --- core/src/main/scala/kafka/admin/AdminUtils.scala https://reviews.apache.org/r/15274/#comment55061 Shall we use null instead of for deleted config values? Sometime in the future we may have with some meanings. - Guozhang Wang On Nov. 6, 2013, 6:13 p.m., Neha Narkhede wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/ --- (Updated Nov. 6, 2013, 6:13 p.m.) Review request for kafka. Bugs: KAFKA-1119 https://issues.apache.org/jira/browse/KAFKA-1119 Repository: kafka Description --- 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 --- Thanks, Neha Narkhede
Re: Review Request 15274: Patch for KAFKA-1119
On Nov. 6, 2013, 6:27 p.m., Guozhang Wang wrote: core/src/main/scala/kafka/admin/AdminUtils.scala, line 219 https://reviews.apache.org/r/15274/diff/2/?file=379571#file379571line219 Shall we use null instead of for deleted config values? Sometime in the future we may have with some meanings. null is not allowed in a Properties object. - Neha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28283 --- On Nov. 6, 2013, 6:13 p.m., Neha Narkhede wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/ --- (Updated Nov. 6, 2013, 6:13 p.m.) Review request for kafka. Bugs: KAFKA-1119 https://issues.apache.org/jira/browse/KAFKA-1119 Repository: kafka Description --- 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 --- Thanks, Neha Narkhede
Re: Review Request 15274: Patch for KAFKA-1119
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28319 --- Ship it! Topic config change as well as create-topic, add-partition, partition-reassignment and preferred leader election are all asynchronous in the sense that the admin command would return immediately and one has to check himself if the process has finished; I think it is better to add an option to make these commands blocking until the process is done. - Guozhang Wang On Nov. 6, 2013, 6:13 p.m., Neha Narkhede wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/ --- (Updated Nov. 6, 2013, 6:13 p.m.) Review request for kafka. Bugs: KAFKA-1119 https://issues.apache.org/jira/browse/KAFKA-1119 Repository: kafka Description --- 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 --- Thanks, Neha Narkhede
Re: Review Request 15274: Patch for KAFKA-1119
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/#review28327 --- It will help to add a --deleteConfig option. We can use it as --config config1=newVal --deleteConfig config2. - Swapnil Ghike On Nov. 6, 2013, 6:13 p.m., Neha Narkhede wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15274/ --- (Updated Nov. 6, 2013, 6:13 p.m.) Review request for kafka. Bugs: KAFKA-1119 https://issues.apache.org/jira/browse/KAFKA-1119 Repository: kafka Description --- 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 --- Thanks, Neha Narkhede
