Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread Neha Narkhede
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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread Neha Narkhede
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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread Neha Narkhede
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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread joel koshy
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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread joel koshy
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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-07 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-06 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-06 Thread 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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-06 Thread Neha Narkhede
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

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-06 Thread 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,

Re: Review Request 15274: Patch for KAFKA-1119

2013-11-06 Thread 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