Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-03-13 Thread Joel Koshy
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18022/ --- (Updated March 13, 2014, 11:03 p.m.) Review request for kafka. Changes

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-03-13 Thread Neha Narkhede
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18022/#review37130 --- Ship it! Minor review comment below

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-03-13 Thread Joel Koshy
On March 13, 2014, 11:25 p.m., Neha Narkhede wrote: core/src/main/scala/kafka/consumer/ConsoleConsumer.scala, line 132 https://reviews.apache.org/r/18022/diff/2-3/?file=511547#file511547line132 Should this default to true if offsets-storage is set to kafka? Right now it is just a

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-03-13 Thread Neha Narkhede
On March 13, 2014, 11:25 p.m., Neha Narkhede wrote: core/src/main/scala/kafka/consumer/ConsoleConsumer.scala, line 132 https://reviews.apache.org/r/18022/diff/2-3/?file=511547#file511547line132 Should this default to true if offsets-storage is set to kafka? Joel Koshy wrote:

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-03-06 Thread Joel Koshy
On March 6, 2014, 1:47 a.m., Neha Narkhede wrote: core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala, line 307 https://reviews.apache.org/r/18022/diff/1-2/?file=483587#file483587line307 ZookeeperConsumerConnector is getting really large. I think it might be

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-03-05 Thread Joel Koshy
On Feb. 28, 2014, 7:30 p.m., Neha Narkhede wrote: core/src/main/scala/kafka/server/KafkaApis.scala, line 234 https://reviews.apache.org/r/18022/diff/1/?file=483590#file483590line234 unused It is used further down. On Feb. 28, 2014, 7:30 p.m., Neha Narkhede wrote:

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-03-05 Thread Joel Koshy
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18022/ --- (Updated March 5, 2014, 11:53 p.m.) Review request for kafka. Changes

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-03-05 Thread Neha Narkhede
On Feb. 28, 2014, 7:30 p.m., Neha Narkhede wrote: core/src/main/scala/kafka/server/OffsetManager.scala, line 149 https://reviews.apache.org/r/18022/diff/1/?file=483593#file483593line149 I wonder if it is safe to assume that the local broker's offsetsTopicNumPartitions matches the

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-03-05 Thread Neha Narkhede
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18022/#review36305 --- Ship it! I think this patch is in reasonable shape and is pretty

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-03-05 Thread Joel Koshy
On March 6, 2014, 1:47 a.m., Neha Narkhede wrote: core/src/main/scala/kafka/client/ClientUtils.scala, line 120 https://reviews.apache.org/r/18022/diff/1-2/?file=483579#file483579line120 Now that you refactored the loop to use while(), is it necessary to use find? It seems like

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-02-28 Thread Neha Narkhede
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18022/#review35746 --- Patch needs to be rebased but I reviewed it as is anyways. Will

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-02-18 Thread Joel Koshy
On Feb. 17, 2014, 6:04 a.m., Tejas Patil wrote: core/src/main/scala/kafka/server/KafkaApis.scala, line 210 https://reviews.apache.org/r/18022/diff/1/?file=483590#file483590line210 For small batch of offset message(s), compression would increase the message size. For a setup,

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-02-18 Thread Joel Koshy
On Feb. 18, 2014, 3:31 a.m., Tejas Patil wrote: core/src/main/scala/kafka/server/OffsetManager.scala, line 338 https://reviews.apache.org/r/18022/diff/1/?file=483593#file483593line338 This is used only for DEBUG. Waste of memory for higher logging levels. Why not allocate /

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-02-18 Thread Joel Koshy
On Feb. 18, 2014, 3:48 a.m., Tejas Patil wrote: My views (which hardly matter !!) about the points in 'Questions/comments for discussion' section: - Change in partition assignment would need sync of offsets across brokers and subsequent bootstrap. Would be better to address in a

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-02-18 Thread Joel Koshy
On Feb. 19, 2014, 1:25 a.m., Guozhang Wang wrote: Some general comments: 1. sbt: do we really want to delete this file in this patch? 2. offset retention minutes: I was not aware of the retention before. I think this is like the consumer heartbeat scheme: the retention

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-02-17 Thread Jun Rao
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18022/#review34549 --- Thanks for the patch. Some comments. 1. KafkaApis: The way that we

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-02-16 Thread Tejas Patil
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18022/#review34615 --- Although I am NOT knowledgeable enough to review Joel's code, did a

Re: Review Request 18022: KAFKA-1012: In-built offset management in Kafka

2014-02-12 Thread Joel Koshy
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18022/ --- (Updated Feb. 12, 2014, 7:50 p.m.) Review request for kafka. Bugs: