Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Plain View
Kafka >> mail # dev >> Re: Review Request 15274: Patch for KAFKA-1119


+
Neha Narkhede 2013-11-07, 16:48
+
Neha Narkhede 2013-11-07, 16:49
+
Neha Narkhede 2013-11-07, 17:36
+
Neha Narkhede 2013-11-06, 18:10
+
Neha Narkhede 2013-11-06, 18:13
+
Neha Narkhede 2013-11-07, 16:50
+
Neha Narkhede 2013-11-07, 17:50
+
joel koshy 2013-11-07, 17:57
+
Neha Narkhede 2013-11-07, 18:19
+
Neha Narkhede 2013-11-07, 18:17
+
joel koshy 2013-11-07, 18:29
Copy link to this message
-
Re: Review Request 15274: Patch for KAFKA-1119


> On Nov. 7, 2013, 6:29 p.m., joel koshy wrote:
> > core/src/main/scala/kafka/admin/AdminUtils.scala, line 224
> > <https://reviews.apache.org/r/15274/diff/4/?file=380481#file380481line224>
> >
> >     Not sure if this is required any more if we're doing validation earlier - since a validation failure would be fatal.

The reason I kept it in the internal API (changeTopicConfigs) is if we use it in places other than tools, it is better to double check the values written to Zookeeper instead of ending up with corrupted topic configs
> On Nov. 7, 2013, 6:29 p.m., joel koshy wrote:
> > core/src/main/scala/kafka/admin/TopicCommand.scala, line 164
> > <https://reviews.apache.org/r/15274/diff/4/?file=380482#file380482line164>
> >
> >     This shouldn't be called for configsToBeDeleted right? - i.e., validation could fail for deleted configs which have no associated value.

Refactored this in the latest patch and simplified the logic. Could you take another look please?
- Neha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15274/#review28393
-----------------------------------------------------------
On Nov. 8, 2013, 1:07 a.m., Neha Narkhede wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15274/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2013, 1:07 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1119
>     https://issues.apache.org/jira/browse/KAFKA-1119
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Improved the patch to simplify the usage of the internal changeTopicConfigs API. It now includes only the final list of configs to be applied to a topic. Any additions/deletions should be done prior to invoking this API
>
>
> Addressed Joel's review comments
>
>
> Add an explicit --deleteConfig option
>
>
> more cleanup
>
>
> Code cleanup
>
>
> Fixed bug that now allows removing all the topic overrides correctly and falling back to the defaults
>
>
> 1. Change the --config to diff the previous configs 2. Add the ability to remove per topic overrides if config is specified without a value
>
>
> Diffs
> -----
>
>   core/src/main/scala/kafka/admin/AdminUtils.scala 8107a64cf1ef1cac763e152bae9f835411c9d3f3
>   core/src/main/scala/kafka/admin/TopicCommand.scala 56f3177e28a34df0ace1d192aef0060cb5e235df
>   core/src/main/scala/kafka/log/LogConfig.scala 51ec796e9e6a10b76daefbd9aea02121fc1d573a
>   core/src/main/scala/kafka/server/TopicConfigManager.scala 56cae58f2a216dcba88b2656fd4a490f11461270
>
> Diff: https://reviews.apache.org/r/15274/diff/
>
>
> Testing
> -------
>
> Locally tested - 1. Adding new config 2. Adding new invalid config 3. Deleting config 4. Deleting all config overrides
>
>
> Thanks,
>
> Neha Narkhede
>
>
 
+
joel koshy 2013-11-08, 01:42
+
joel koshy 2013-11-07, 18:31
+
Neha Narkhede 2013-11-08, 01:10
+
joel koshy 2013-11-08, 01:42
+
Neha Narkhede 2013-11-08, 01:07
+
joel koshy 2013-11-08, 01:42
+
Neha Narkhede 2013-11-06, 18:37