Re: Review Request 15274: Patch for KAFKA-1119

[joel koshy]

---
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

[joel koshy]

> On Nov. 7, 2013, 6:29 p.m., joel koshy wrote:
> > core/src/main/scala/kafka/admin/AdminUtils.scala, line 224
> > 
> >
> > 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

[joel koshy]

> On Nov. 7, 2013, 6:31 p.m., joel koshy wrote:
> > core/src/main/scala/kafka/admin/TopicCommand.scala, line 153
> > 
> >
> > 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

[Neha Narkhede]

> On Nov. 7, 2013, 6:31 p.m., joel koshy wrote:
> > core/src/main/scala/kafka/admin/TopicCommand.scala, line 153
> > 
> >
> > 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

[Neha Narkhede]

> On Nov. 7, 2013, 6:29 p.m., joel koshy wrote:
> > core/src/main/scala/kafka/admin/AdminUtils.scala, line 224
> > 
> >
> > 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
> > 
> >
> > 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

[Neha Narkhede]

---
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

[Swapnil Ghike]

---
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


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

[joel koshy]

---
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


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

[joel koshy]

---
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


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


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

[Neha Narkhede]

> On Nov. 7, 2013, 5:57 p.m., joel koshy wrote:
> > core/src/main/scala/kafka/admin/TopicCommand.scala, line 153
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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

[Neha Narkhede]

---
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

[joel koshy]

---
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


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


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


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

[Guozhang Wang]

> 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

[Neha Narkhede]

---
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

[Neha Narkhede]

> 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

[Neha Narkhede]

---
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

[Neha Narkhede]

> 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

[Neha Narkhede]

> 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

[Swapnil Ghike]

---
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

[Guozhang Wang]

---
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

[Neha Narkhede]

> On Nov. 6, 2013, 6:27 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/admin/AdminUtils.scala, line 219
> > 
> >
> > 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

[Guozhang Wang]

---
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


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

[Neha Narkhede]

---
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