Review Request 27684: Patch for KAFKA-1743

2014-11-06 Thread Manikumar Reddy O 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/
---

Review request for kafka.


Bugs: KAFKA-1743
https://issues.apache.org/jira/browse/KAFKA-1743


Repository: kafka


Description
---

def commitOffsets method added to make ConsumerConnector backward  compatible


Diffs
-

  core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
07677c1c26768ef9c9032626180d0015f12cb0e0 
  core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 
fbc680fde21b02f11285a4f4b442987356abd17b 

Diff: https://reviews.apache.org/r/27684/diff/


Testing
---


Thanks,

Manikumar Reddy O

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-07 Thread Gwen Shapira 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review60364
---



core/src/main/scala/kafka/consumer/ConsumerConnector.scala


I'd add a comment on why we have two interfaces here, since its 
none-obvious and someone may try to refactor in the future... Perhaps link to 
the email thread?


- Gwen Shapira


On Nov. 6, 2014, 5:13 p.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 6, 2014, 5:13 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  compatible
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
>   core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 
> fbc680fde21b02f11285a4f4b442987356abd17b 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-07 Thread Joel Koshy 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review60375
---



core/src/main/scala/kafka/consumer/ConsumerConnector.scala


Can we instead do:

def commitOffsets { commitOffsets(false) }

so all implementations of the trait don't need to do the above.


- Joel Koshy


On Nov. 6, 2014, 5:13 p.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 6, 2014, 5:13 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  compatible
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
>   core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 
> fbc680fde21b02f11285a4f4b442987356abd17b 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-07 Thread Manikumar Reddy O 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/
---

(Updated Nov. 8, 2014, 6:20 a.m.)


Review request for kafka.


Bugs: KAFKA-1743
https://issues.apache.org/jira/browse/KAFKA-1743


Repository: kafka


Description
---

def commitOffsets method added to make ConsumerConnector backward  compatible


Diffs (updated)
-

  core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
07677c1c26768ef9c9032626180d0015f12cb0e0 

Diff: https://reviews.apache.org/r/27684/diff/


Testing
---


Thanks,

Manikumar Reddy O

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-07 Thread Manikumar Reddy O 


> On Nov. 7, 2014, 7:35 p.m., Gwen Shapira wrote:
> > core/src/main/scala/kafka/consumer/ConsumerConnector.scala, line 73
> > 
> >
> > I'd add a comment on why we have two interfaces here, since its 
> > none-obvious and someone may try to refactor in the future... Perhaps link 
> > to the email thread?

Agreed. Added a comment.


- Manikumar Reddy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review60364
---


On Nov. 8, 2014, 6:20 a.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 8, 2014, 6:20 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  compatible
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-10 Thread Jun Rao 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review60652
---


Thanks for the patch. Some comments below.


core/src/main/scala/kafka/consumer/ConsumerConnector.scala


We will also need to change the interface in ConsumerConnector from 

  def commitOffsets(retryOnFailure: Boolean = true)
  
back to 

  def commitOffsets
  
In ZookeeperConsumerconnector, we can make the following method private

def commitOffsets(retryOnFailure: Boolean = true)

Another question, will scala compiler be confused with 2 methods, one w/o 
parenthsis and one with 1 parameter having a default? Could you try compiling 
the code on all scala versions?


- Jun Rao


On Nov. 8, 2014, 6:20 a.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 8, 2014, 6:20 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  compatible
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-12 Thread Manikumar Reddy O 


> On Nov. 10, 2014, 7:50 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/ConsumerConnector.scala, lines 76-80
> > 
> >
> > We will also need to change the interface in ConsumerConnector from 
> > 
> >   def commitOffsets(retryOnFailure: Boolean = true)
> >   
> > back to 
> > 
> >   def commitOffsets
> >   
> > In ZookeeperConsumerconnector, we can make the following method private
> > 
> > def commitOffsets(retryOnFailure: Boolean = true)
> > 
> > Another question, will scala compiler be confused with 2 methods, one 
> > w/o parenthsis and one with 1 parameter having a default? Could you try 
> > compiling the code on all scala versions?

Currently below classes uses the new method  commitOffsets(true). 

kafka/javaapi/consumer/ZookeeperConsumerConnector.scala
kafka/tools/TestEndToEndLatency.scala

If we are changing the interface,  then we need to change the above classes 
also. 

If we are not fixing this on trunk, then same problem will come in 0.8.3. 
How to handle this? 

2 methods, one w/o parenthsis and one with 1 parameter is getting compiled on 
all scala versions.


- Manikumar Reddy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review60652
---


On Nov. 8, 2014, 6:20 a.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 8, 2014, 6:20 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  compatible
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-12 Thread Jun Rao 


> On Nov. 10, 2014, 7:50 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/ConsumerConnector.scala, lines 76-80
> > 
> >
> > We will also need to change the interface in ConsumerConnector from 
> > 
> >   def commitOffsets(retryOnFailure: Boolean = true)
> >   
> > back to 
> > 
> >   def commitOffsets
> >   
> > In ZookeeperConsumerconnector, we can make the following method private
> > 
> > def commitOffsets(retryOnFailure: Boolean = true)
> > 
> > Another question, will scala compiler be confused with 2 methods, one 
> > w/o parenthsis and one with 1 parameter having a default? Could you try 
> > compiling the code on all scala versions?
> 
> Manikumar Reddy O wrote:
> Currently below classes uses the new method  commitOffsets(true). 
> 
> kafka/javaapi/consumer/ZookeeperConsumerConnector.scala
> kafka/tools/TestEndToEndLatency.scala
> 
> If we are changing the interface,  then we need to change the above 
> classes 
> also. 
> 
> If we are not fixing this on trunk, then same problem will come in 0.8.3. 
> How to handle this? 
> 
> 2 methods, one w/o parenthsis and one with 1 parameter is getting 
> compiled on 
> all scala versions.

Thanks for the explanation. There is actually a bit of inconsistency introduced 
in this patch. 

In kafka.javaapi.consumer.ZookeeperConsumerConnector, commitOffsets() is 
implemented as the following.
  def commitOffsets() {
underlying.commitOffsets()
  }
This actually calls underlying.commitOffsets(isAutoCommit: Boolean = true) with 
a default value of true. However, ConsumerConnector.commitOffset is implemented 
as the following which sets isAutoCommit to false.
  def commitOffsets { commitOffsets(false) }
  
So, we should use true in the above.

Another thing that I was thinking is that it's going to be a bit confusing if 
we have the following scala apis.
  def commitOffsets(retryOnFailure: Boolean = true)
  def commitOffsets
  
So, if you do commitOffset it calls the second one and if you do 
commitOffset(), you actually call the first one. However, the expectation is 
probably the same method will be called in both cases. Would it be better if we 
get rid of the default like the following? Then, it's clear which method will 
be called.
  def commitOffsets(retryOnFailure: Boolean)
  def commitOffsets


- Jun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review60652
---


On Nov. 8, 2014, 6:20 a.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 8, 2014, 6:20 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  compatible
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-13 Thread Manikumar Reddy O 


> On Nov. 10, 2014, 7:50 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/ConsumerConnector.scala, lines 76-80
> > 
> >
> > We will also need to change the interface in ConsumerConnector from 
> > 
> >   def commitOffsets(retryOnFailure: Boolean = true)
> >   
> > back to 
> > 
> >   def commitOffsets
> >   
> > In ZookeeperConsumerconnector, we can make the following method private
> > 
> > def commitOffsets(retryOnFailure: Boolean = true)
> > 
> > Another question, will scala compiler be confused with 2 methods, one 
> > w/o parenthsis and one with 1 parameter having a default? Could you try 
> > compiling the code on all scala versions?
> 
> Manikumar Reddy O wrote:
> Currently below classes uses the new method  commitOffsets(true). 
> 
> kafka/javaapi/consumer/ZookeeperConsumerConnector.scala
> kafka/tools/TestEndToEndLatency.scala
> 
> If we are changing the interface,  then we need to change the above 
> classes 
> also. 
> 
> If we are not fixing this on trunk, then same problem will come in 0.8.3. 
> How to handle this? 
> 
> 2 methods, one w/o parenthsis and one with 1 parameter is getting 
> compiled on 
> all scala versions.
> 
> Jun Rao wrote:
> Thanks for the explanation. There is actually a bit of inconsistency 
> introduced in this patch. 
> 
> In kafka.javaapi.consumer.ZookeeperConsumerConnector, commitOffsets() is 
> implemented as the following.
>   def commitOffsets() {
> underlying.commitOffsets()
>   }
> This actually calls underlying.commitOffsets(isAutoCommit: Boolean = 
> true) with a default value of true. However, ConsumerConnector.commitOffset 
> is implemented as the following which sets isAutoCommit to false.
>   def commitOffsets { commitOffsets(false) }
>   
> So, we should use true in the above.
> 
> Another thing that I was thinking is that it's going to be a bit 
> confusing if we have the following scala apis.
>   def commitOffsets(retryOnFailure: Boolean = true)
>   def commitOffsets
>   
> So, if you do commitOffset it calls the second one and if you do 
> commitOffset(), you actually call the first one. However, the expectation is 
> probably the same method will be called in both cases. Would it be better if 
> we get rid of the default like the following? Then, it's clear which method 
> will be called.
>   def commitOffsets(retryOnFailure: Boolean)
>   def commitOffsets

This JIRA is to make ConsumerConnecor compatible with 0.8.1, right?  then, we 
need to remove   
"def commitOffsets(retryOnFailure: Boolean = true)" from ConsumerConnecor.

Changing the API to "def commitOffsets(retryOnFailure: Boolean)" will not help 
us. 
It still breaks the compatability.


- Manikumar Reddy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review60652
---


On Nov. 8, 2014, 6:20 a.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 8, 2014, 6:20 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  compatible
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-13 Thread Jun Rao 


> On Nov. 10, 2014, 7:50 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/ConsumerConnector.scala, lines 76-80
> > 
> >
> > We will also need to change the interface in ConsumerConnector from 
> > 
> >   def commitOffsets(retryOnFailure: Boolean = true)
> >   
> > back to 
> > 
> >   def commitOffsets
> >   
> > In ZookeeperConsumerconnector, we can make the following method private
> > 
> > def commitOffsets(retryOnFailure: Boolean = true)
> > 
> > Another question, will scala compiler be confused with 2 methods, one 
> > w/o parenthsis and one with 1 parameter having a default? Could you try 
> > compiling the code on all scala versions?
> 
> Manikumar Reddy O wrote:
> Currently below classes uses the new method  commitOffsets(true). 
> 
> kafka/javaapi/consumer/ZookeeperConsumerConnector.scala
> kafka/tools/TestEndToEndLatency.scala
> 
> If we are changing the interface,  then we need to change the above 
> classes 
> also. 
> 
> If we are not fixing this on trunk, then same problem will come in 0.8.3. 
> How to handle this? 
> 
> 2 methods, one w/o parenthsis and one with 1 parameter is getting 
> compiled on 
> all scala versions.
> 
> Jun Rao wrote:
> Thanks for the explanation. There is actually a bit of inconsistency 
> introduced in this patch. 
> 
> In kafka.javaapi.consumer.ZookeeperConsumerConnector, commitOffsets() is 
> implemented as the following.
>   def commitOffsets() {
> underlying.commitOffsets()
>   }
> This actually calls underlying.commitOffsets(isAutoCommit: Boolean = 
> true) with a default value of true. However, ConsumerConnector.commitOffset 
> is implemented as the following which sets isAutoCommit to false.
>   def commitOffsets { commitOffsets(false) }
>   
> So, we should use true in the above.
> 
> Another thing that I was thinking is that it's going to be a bit 
> confusing if we have the following scala apis.
>   def commitOffsets(retryOnFailure: Boolean = true)
>   def commitOffsets
>   
> So, if you do commitOffset it calls the second one and if you do 
> commitOffset(), you actually call the first one. However, the expectation is 
> probably the same method will be called in both cases. Would it be better if 
> we get rid of the default like the following? Then, it's clear which method 
> will be called.
>   def commitOffsets(retryOnFailure: Boolean)
>   def commitOffsets
> 
> Manikumar Reddy O wrote:
> This JIRA is to make ConsumerConnecor compatible with 0.8.1, right?  
> then, we need to remove   
> "def commitOffsets(retryOnFailure: Boolean = true)" from ConsumerConnecor.
> 
> Changing the API to "def commitOffsets(retryOnFailure: Boolean)" will not 
> help us. 
> It still breaks the compatability.

In 0.8.1, ConsumerConnector has
  def commitOffsets

I was thinking of having the following two APIs in ConsumerConnector in 0.8.2. 
That should be backward compatible with the 0.8.1 api, right?
  def commitOffsets(retryOnFailure: Boolean)
  def commitOffsets


- Jun


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review60652
---


On Nov. 8, 2014, 6:20 a.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 8, 2014, 6:20 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  compatible
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-14 Thread Manikumar Reddy O 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/
---

(Updated Nov. 14, 2014, 5 p.m.)


Review request for kafka.


Bugs: KAFKA-1743
https://issues.apache.org/jira/browse/KAFKA-1743


Repository: kafka


Description (updated)
---

def commitOffsets method added to make ConsumerConnector backward  compatible; 
Adressing Jun's comments


Diffs (updated)
-

  core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
07677c1c26768ef9c9032626180d0015f12cb0e0 

Diff: https://reviews.apache.org/r/27684/diff/


Testing
---


Thanks,

Manikumar Reddy O

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-14 Thread Manikumar Reddy O 


> On Nov. 10, 2014, 7:50 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/ConsumerConnector.scala, lines 76-80
> > 
> >
> > We will also need to change the interface in ConsumerConnector from 
> > 
> >   def commitOffsets(retryOnFailure: Boolean = true)
> >   
> > back to 
> > 
> >   def commitOffsets
> >   
> > In ZookeeperConsumerconnector, we can make the following method private
> > 
> > def commitOffsets(retryOnFailure: Boolean = true)
> > 
> > Another question, will scala compiler be confused with 2 methods, one 
> > w/o parenthsis and one with 1 parameter having a default? Could you try 
> > compiling the code on all scala versions?
> 
> Manikumar Reddy O wrote:
> Currently below classes uses the new method  commitOffsets(true). 
> 
> kafka/javaapi/consumer/ZookeeperConsumerConnector.scala
> kafka/tools/TestEndToEndLatency.scala
> 
> If we are changing the interface,  then we need to change the above 
> classes 
> also. 
> 
> If we are not fixing this on trunk, then same problem will come in 0.8.3. 
> How to handle this? 
> 
> 2 methods, one w/o parenthsis and one with 1 parameter is getting 
> compiled on 
> all scala versions.
> 
> Jun Rao wrote:
> Thanks for the explanation. There is actually a bit of inconsistency 
> introduced in this patch. 
> 
> In kafka.javaapi.consumer.ZookeeperConsumerConnector, commitOffsets() is 
> implemented as the following.
>   def commitOffsets() {
> underlying.commitOffsets()
>   }
> This actually calls underlying.commitOffsets(isAutoCommit: Boolean = 
> true) with a default value of true. However, ConsumerConnector.commitOffset 
> is implemented as the following which sets isAutoCommit to false.
>   def commitOffsets { commitOffsets(false) }
>   
> So, we should use true in the above.
> 
> Another thing that I was thinking is that it's going to be a bit 
> confusing if we have the following scala apis.
>   def commitOffsets(retryOnFailure: Boolean = true)
>   def commitOffsets
>   
> So, if you do commitOffset it calls the second one and if you do 
> commitOffset(), you actually call the first one. However, the expectation is 
> probably the same method will be called in both cases. Would it be better if 
> we get rid of the default like the following? Then, it's clear which method 
> will be called.
>   def commitOffsets(retryOnFailure: Boolean)
>   def commitOffsets
> 
> Manikumar Reddy O wrote:
> This JIRA is to make ConsumerConnecor compatible with 0.8.1, right?  
> then, we need to remove   
> "def commitOffsets(retryOnFailure: Boolean = true)" from ConsumerConnecor.
> 
> Changing the API to "def commitOffsets(retryOnFailure: Boolean)" will not 
> help us. 
> It still breaks the compatability.
> 
> Jun Rao wrote:
> In 0.8.1, ConsumerConnector has
>   def commitOffsets
> 
> I was thinking of having the following two APIs in ConsumerConnector in 
> 0.8.2. That should be backward compatible with the 0.8.1 api, right?
>   def commitOffsets(retryOnFailure: Boolean)
>   def commitOffsets

Ok. I was thinking there may be some custom implementations of 
ConsumerConnector interface out side the kafka codebase. So changing the 
interface will break those implementations. 

I added the following APIs in ConsumerConnector.
  def commitOffsets(retryOnFailure: Boolean)
  def commitOffsets


- Manikumar Reddy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review60652
---


On Nov. 14, 2014, 5 p.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 14, 2014, 5 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  
> compatible; Adressing Jun's comments
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-14 Thread Jun Rao 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review61486
---


Thanks for the patch. For clarity, in ZookeeperConsumerConnector, instead of 
having the following,
  def commitOffsets(isAutoCommit: Boolean = true) 

could we break it into two separate methods, same as what's defined in 
ConsumerConnector?

- Jun Rao


On Nov. 14, 2014, 5 p.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 14, 2014, 5 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  
> compatible; Adressing Jun's comments
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-15 Thread Manikumar Reddy O 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/
---

(Updated Nov. 16, 2014, 6:42 a.m.)


Review request for kafka.


Bugs: KAFKA-1743
https://issues.apache.org/jira/browse/KAFKA-1743


Repository: kafka


Description (updated)
---

def commitOffsets method added to make ConsumerConnector backward  compatible; 
Addressing Jun's comments


Diffs (updated)
-

  core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
07677c1c26768ef9c9032626180d0015f12cb0e0 
  core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 
f476973eeff653473a60c3ecf36e870e386536bc 
  core/src/main/scala/kafka/javaapi/consumer/ZookeeperConsumerConnector.scala 
1f98db5d692adc113189ec8c75a4fad29d6b6ffe 

Diff: https://reviews.apache.org/r/27684/diff/


Testing
---


Thanks,

Manikumar Reddy O

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-15 Thread Manikumar Reddy O 


> On Nov. 14, 2014, 7:29 p.m., Jun Rao wrote:
> > Thanks for the patch. For clarity, in ZookeeperConsumerConnector, instead 
> > of having the following,
> >   def commitOffsets(isAutoCommit: Boolean = true) 
> > 
> > could we break it into two separate methods, same as what's defined in 
> > ConsumerConnector?

Done. Added the following APIs in ZookeeperConsumerConnector.

def commitOffsets(retryOnFailure: Boolean)
def commitOffsets


- Manikumar Reddy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review61486
---


On Nov. 16, 2014, 6:42 a.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 16, 2014, 6:42 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  
> compatible; Addressing Jun's comments
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
>   core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 
> f476973eeff653473a60c3ecf36e870e386536bc 
>   core/src/main/scala/kafka/javaapi/consumer/ZookeeperConsumerConnector.scala 
> 1f98db5d692adc113189ec8c75a4fad29d6b6ffe 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-17 Thread Jun Rao 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review61863
---


Thanks for the patch. Got the following compilation error.

:core:compileTestScala/Users/junrao/intellij/kafka/core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala:116:
 overloaded method value commitOffsets with alternatives:
  => Unit 
  (isAutoCommit: Boolean)Unit
 cannot be applied to ()
zkConsumerConnector1.commitOffsets()
 ^
/Users/junrao/intellij/kafka/core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala:204:
 overloaded method value commitOffsets with alternatives:
  => Unit 
  (isAutoCommit: Boolean)Unit
 cannot be applied to ()
zkConsumerConnector1.commitOffsets()
 ^
two errors found
 FAILED

- Jun Rao


On Nov. 16, 2014, 6:42 a.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 16, 2014, 6:42 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  
> compatible; Addressing Jun's comments
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
>   core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 
> f476973eeff653473a60c3ecf36e870e386536bc 
>   core/src/main/scala/kafka/javaapi/consumer/ZookeeperConsumerConnector.scala 
> 1f98db5d692adc113189ec8c75a4fad29d6b6ffe 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-17 Thread Manikumar Reddy O 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/
---

(Updated Nov. 18, 2014, 5:29 a.m.)


Review request for kafka.


Bugs: KAFKA-1743
https://issues.apache.org/jira/browse/KAFKA-1743


Repository: kafka


Description
---

def commitOffsets method added to make ConsumerConnector backward  compatible; 
Addressing Jun's comments


Diffs (updated)
-

  core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
07677c1c26768ef9c9032626180d0015f12cb0e0 
  core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 
fe9d8e028cf08db844f0d72de4dd1e78f0e4258c 
  core/src/main/scala/kafka/javaapi/consumer/ZookeeperConsumerConnector.scala 
1f98db5d692adc113189ec8c75a4fad29d6b6ffe 
  core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala 
bad099a904967651bc3a38b6bb9a9cdb592b832b 

Diff: https://reviews.apache.org/r/27684/diff/


Testing
---


Thanks,

Manikumar Reddy O

Re: Review Request 27684: Patch for KAFKA-1743

2014-11-17 Thread Manikumar Reddy O 


> On Nov. 18, 2014, 2:49 a.m., Jun Rao wrote:
> > Thanks for the patch. Got the following compilation error.
> > 
> > :core:compileTestScala/Users/junrao/intellij/kafka/core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala:116:
> >  overloaded method value commitOffsets with alternatives:
> >   => Unit 
> >   (isAutoCommit: Boolean)Unit
> >  cannot be applied to ()
> > zkConsumerConnector1.commitOffsets()
> >  ^
> > /Users/junrao/intellij/kafka/core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala:204:
> >  overloaded method value commitOffsets with alternatives:
> >   => Unit 
> >   (isAutoCommit: Boolean)Unit
> >  cannot be applied to ()
> > zkConsumerConnector1.commitOffsets()
> >  ^
> > two errors found
> >  FAILED

Oh My Bad!  missed the test classes. Pl review the latest patch.


- Manikumar Reddy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27684/#review61863
---


On Nov. 18, 2014, 5:29 a.m., Manikumar Reddy O wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27684/
> ---
> 
> (Updated Nov. 18, 2014, 5:29 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-1743
> https://issues.apache.org/jira/browse/KAFKA-1743
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> def commitOffsets method added to make ConsumerConnector backward  
> compatible; Addressing Jun's comments
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/consumer/ConsumerConnector.scala 
> 07677c1c26768ef9c9032626180d0015f12cb0e0 
>   core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 
> fe9d8e028cf08db844f0d72de4dd1e78f0e4258c 
>   core/src/main/scala/kafka/javaapi/consumer/ZookeeperConsumerConnector.scala 
> 1f98db5d692adc113189ec8c75a4fad29d6b6ffe 
>   
> core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala 
> bad099a904967651bc3a38b6bb9a9cdb592b832b 
> 
> Diff: https://reviews.apache.org/r/27684/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Manikumar Reddy O
> 
>