kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Manikumar Reddy O" <manikumar.re...@gmail.com>
Subject Re: Review Request 27684: Patch for KAFKA-1743
Date Fri, 14 Nov 2014 17:15:58 GMT


> On Nov. 10, 2014, 7:50 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/ConsumerConnector.scala, lines 76-80
> > <https://reviews.apache.org/r/27684/diff/2/?file=755292#file755292line76>
> >
> >     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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message