You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Neha Narkhede <ne...@gmail.com> on 2013/11/06 19:10:33 UTC
Review Request 15274: Patch for KAFKA-1119
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15274/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-1119
https://issues.apache.org/jira/browse/KAFKA-1119
Repository: kafka
Description
-------
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
Posted by Neha Narkhede <ne...@gmail.com>.
> 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
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
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
Posted by Guozhang Wang <gu...@linkedin.com>.
> 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
Posted by Neha Narkhede <ne...@gmail.com>.
> 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
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
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
Posted by Neha Narkhede <ne...@gmail.com>.
> 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.
Like we discussed offline, deleteConfig complicates the ability to accept configs easily in bulk and apply those.
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15274/#review28327
-----------------------------------------------------------
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
Posted by Neha Narkhede <ne...@gmail.com>.
> 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
Posted by Swapnil Ghike <sg...@linkedin.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 15274: Patch for KAFKA-1119
Posted by Neha Narkhede <ne...@gmail.com>.
> 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
>
>
Re: Review Request 15274: Patch for KAFKA-1119
Posted by joel koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15274/#review28385
-----------------------------------------------------------
core/src/main/scala/kafka/admin/AdminUtils.scala
<https://reviews.apache.org/r/15274/#comment55214>
Would it be better to do validation of all configs (to be removed and altered) earlier - i.e., in parseTopicConfigs?
core/src/main/scala/kafka/admin/TopicCommand.scala
<https://reviews.apache.org/r/15274/#comment55206>
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.
core/src/main/scala/kafka/admin/TopicCommand.scala
<https://reviews.apache.org/r/15274/#comment55207>
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.
- joel koshy
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
Posted by joel koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
Posted by joel koshy <jj...@gmail.com>.
> 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
Posted by Neha Narkhede <ne...@gmail.com>.
> 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
Posted by joel koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Swapnil Ghike <sg...@linkedin.com>.
-----------------------------------------------------------
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
Posted by Neha Narkhede <ne...@gmail.com>.
> 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
Posted by joel koshy <jj...@gmail.com>.
> 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
Posted by joel koshy <jj...@gmail.com>.
-----------------------------------------------------------
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Still need reviews from committers.
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 (updated)
-------
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
Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
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