> 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.
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 > >
