You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jun Rao <ju...@gmail.com> on 2015/01/08 02:54:06 UTC

Review Request 29692: Patch for kafka-1841

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

Review request for kafka.


Bugs: kafka-1841
    https://issues.apache.org/jira/browse/kafka-1841


Repository: kafka


Description
-------

remove timestamp from version 0 protocol


Diffs
-----

  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 3ee5cbad55ce836fd04bb954dcf6ef2f9bc3288f 
  core/src/main/scala/kafka/api/OffsetCommitRequest.scala 861a6cf11dc6b6431fcbbe9de00c74a122f204bd 
  core/src/main/scala/kafka/common/OffsetMetadataAndError.scala 4cabffeacea09a49913505db19a96a55d58c0909 
  core/src/main/scala/kafka/server/KafkaApis.scala 2f009920af37de3cf0a3eb131f2124f4e532c4e4 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 

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


Testing
-------


Thanks,

Jun Rao

Re: Review Request 29692: Patch for kafka-1841

Posted by Jun Rao <ju...@gmail.com>.

> On Jan. 12, 2015, 7:47 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, line 88
> > <https://reviews.apache.org/r/29692/diff/3/?file=815275#file815275line88>
> >
> >     We should probably also change OffsetCommitRequest.responseFor . The issue is that if you get an UnknownTopicOrPartition error right now we convert that to a ConsumerCoordinatorNotAvailableCode which does not apply for v0.
> >     
> >     (BTW, if this patch were for trunk you would not need to do this since latest trunk sets the code correctly in the OffsetManager class)
> >     
> >     Alternatively, we could just remove the check here but that would be a change in behavior.

This doesn't seem to be necessary since in v0, we handle UnknownTopicOrPartition explicitly in handleOffsetCommitRequest() by converting the exception to the right error code.


> On Jan. 12, 2015, 7:47 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, line 101
> > <https://reviews.apache.org/r/29692/diff/3/?file=815275#file815275line101>
> >
> >     Should we explicitly version the scala side of OffsetCommitResponse as well especially given that the Java version has a v0/v1? E.g., if a client proactively checks for the response version... This seems to always send back version = 0 in the response

Added a comment to indicate that this constructor is for both v0 and v1.


- Jun


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


On Jan. 12, 2015, 10:30 p.m., Jun Rao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29692/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 10:30 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: kafka-1841
>     https://issues.apache.org/jira/browse/kafka-1841
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> addressing Joel's comments
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 3ee5cbad55ce836fd04bb954dcf6ef2f9bc3288f 
>   core/src/main/scala/kafka/api/OffsetCommitRequest.scala 861a6cf11dc6b6431fcbbe9de00c74a122f204bd 
>   core/src/main/scala/kafka/api/OffsetCommitResponse.scala 624a1c1cc540688ae2b1fb96665696a6084158e5 
>   core/src/main/scala/kafka/api/OffsetFetchRequest.scala c7604b9cdeb8f46507f04ed7d35fcb3d5f150713 
>   core/src/main/scala/kafka/server/KafkaApis.scala 9a61fcba3b5eeb295676b3ef720c719ef5244642 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
> 
> Diff: https://reviews.apache.org/r/29692/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jun Rao
> 
>

Re: Review Request 29692: Patch for kafka-1841

Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29692/#review67667
-----------------------------------------------------------


Thanks for patching this. Looks good overall.


clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java
<https://reviews.apache.org/r/29692/#comment111737>

    



core/src/main/scala/kafka/common/OffsetMetadataAndError.scala
<https://reviews.apache.org/r/29692/#comment111740>

    rather than use a var can we just use a case class copy to modify? i.e.,
    `modifiedInstance = originalInstance.copy(fieldToModify=modifiedValue)`



core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/29692/#comment111748>

    We should probably also change OffsetCommitRequest.responseFor . The issue is that if you get an UnknownTopicOrPartition error right now we convert that to a ConsumerCoordinatorNotAvailableCode which does not apply for v0.
    
    (BTW, if this patch were for trunk you would not need to do this since latest trunk sets the code correctly in the OffsetManager class)
    
    Alternatively, we could just remove the check here but that would be a change in behavior.



core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/29692/#comment111747>

    Should we explicitly version the scala side of OffsetCommitResponse as well especially given that the Java version has a v0/v1? E.g., if a client proactively checks for the response version... This seems to always send back version = 0 in the response



core/src/main/scala/kafka/server/KafkaApis.scala
<https://reviews.apache.org/r/29692/#comment111757>

    Similar comment as above on the response version.


- Joel Koshy


On Jan. 9, 2015, 10:36 p.m., Jun Rao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29692/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2015, 10:36 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: kafka-1841
>     https://issues.apache.org/jira/browse/kafka-1841
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> rebased
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 3ee5cbad55ce836fd04bb954dcf6ef2f9bc3288f 
>   core/src/main/scala/kafka/api/OffsetCommitRequest.scala 861a6cf11dc6b6431fcbbe9de00c74a122f204bd 
>   core/src/main/scala/kafka/api/OffsetFetchRequest.scala c7604b9cdeb8f46507f04ed7d35fcb3d5f150713 
>   core/src/main/scala/kafka/common/OffsetMetadataAndError.scala 4cabffeacea09a49913505db19a96a55d58c0909 
>   core/src/main/scala/kafka/server/KafkaApis.scala 9a61fcba3b5eeb295676b3ef720c719ef5244642 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
> 
> Diff: https://reviews.apache.org/r/29692/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jun Rao
> 
>

Re: Review Request 29692: Patch for kafka-1841

Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29692/#review67775
-----------------------------------------------------------

Ship it!


Reversed an earlier comment, but up to you to pick which one you prefer.


core/src/main/scala/kafka/api/OffsetCommitRequest.scala
<https://reviews.apache.org/r/29692/#comment111848>

    Hmm.. actually looking over this I think what you had earlier looks better overall. i.e., maybe changing the field to a var is better? Sorry about that!


- Joel Koshy


On Jan. 12, 2015, 10:30 p.m., Jun Rao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29692/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2015, 10:30 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: kafka-1841
>     https://issues.apache.org/jira/browse/kafka-1841
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> addressing Joel's comments
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 3ee5cbad55ce836fd04bb954dcf6ef2f9bc3288f 
>   core/src/main/scala/kafka/api/OffsetCommitRequest.scala 861a6cf11dc6b6431fcbbe9de00c74a122f204bd 
>   core/src/main/scala/kafka/api/OffsetCommitResponse.scala 624a1c1cc540688ae2b1fb96665696a6084158e5 
>   core/src/main/scala/kafka/api/OffsetFetchRequest.scala c7604b9cdeb8f46507f04ed7d35fcb3d5f150713 
>   core/src/main/scala/kafka/server/KafkaApis.scala 9a61fcba3b5eeb295676b3ef720c719ef5244642 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
> 
> Diff: https://reviews.apache.org/r/29692/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jun Rao
> 
>

Re: Review Request 29692: Patch for kafka-1841

Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29692/
-----------------------------------------------------------

(Updated Jan. 12, 2015, 10:30 p.m.)


Review request for kafka.


Bugs: kafka-1841
    https://issues.apache.org/jira/browse/kafka-1841


Repository: kafka


Description (updated)
-------

addressing Joel's comments


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 3ee5cbad55ce836fd04bb954dcf6ef2f9bc3288f 
  core/src/main/scala/kafka/api/OffsetCommitRequest.scala 861a6cf11dc6b6431fcbbe9de00c74a122f204bd 
  core/src/main/scala/kafka/api/OffsetCommitResponse.scala 624a1c1cc540688ae2b1fb96665696a6084158e5 
  core/src/main/scala/kafka/api/OffsetFetchRequest.scala c7604b9cdeb8f46507f04ed7d35fcb3d5f150713 
  core/src/main/scala/kafka/server/KafkaApis.scala 9a61fcba3b5eeb295676b3ef720c719ef5244642 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 

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


Testing
-------


Thanks,

Jun Rao

Re: Review Request 29692: Patch for kafka-1841

Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29692/
-----------------------------------------------------------

(Updated Jan. 9, 2015, 10:36 p.m.)


Review request for kafka.


Bugs: kafka-1841
    https://issues.apache.org/jira/browse/kafka-1841


Repository: kafka


Description (updated)
-------

rebased


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 3ee5cbad55ce836fd04bb954dcf6ef2f9bc3288f 
  core/src/main/scala/kafka/api/OffsetCommitRequest.scala 861a6cf11dc6b6431fcbbe9de00c74a122f204bd 
  core/src/main/scala/kafka/api/OffsetFetchRequest.scala c7604b9cdeb8f46507f04ed7d35fcb3d5f150713 
  core/src/main/scala/kafka/common/OffsetMetadataAndError.scala 4cabffeacea09a49913505db19a96a55d58c0909 
  core/src/main/scala/kafka/server/KafkaApis.scala 9a61fcba3b5eeb295676b3ef720c719ef5244642 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 

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


Testing
-------


Thanks,

Jun Rao

Re: Review Request 29692: Patch for kafka-1841

Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29692/
-----------------------------------------------------------

(Updated Jan. 8, 2015, 11:07 p.m.)


Review request for kafka.


Bugs: kafka-1841
    https://issues.apache.org/jira/browse/kafka-1841


Repository: kafka


Description (updated)
-------

version 0 of fetchOffset reads from ZK and version 1 of fetchOffset reads from Kafka


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
  clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 3ee5cbad55ce836fd04bb954dcf6ef2f9bc3288f 
  core/src/main/scala/kafka/api/OffsetCommitRequest.scala 861a6cf11dc6b6431fcbbe9de00c74a122f204bd 
  core/src/main/scala/kafka/api/OffsetFetchRequest.scala c7604b9cdeb8f46507f04ed7d35fcb3d5f150713 
  core/src/main/scala/kafka/common/OffsetMetadataAndError.scala 4cabffeacea09a49913505db19a96a55d58c0909 
  core/src/main/scala/kafka/server/KafkaApis.scala 2f009920af37de3cf0a3eb131f2124f4e532c4e4 
  core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 

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


Testing
-------


Thanks,

Jun Rao

Re: Review Request 29692: Patch for kafka-1841

Posted by Jun Rao <ju...@gmail.com>.

> On Jan. 8, 2015, 2:15 a.m., Neha Narkhede wrote:
> > Jun, have you had a chance to test the 0.8.1 client against the 0.8.2 server, as we had discussed earlier today?

Yes, it works as expected with the following test program built with the 0.8.1 branch.

package kafka

import kafka.utils.Logging
import kafka.consumer.SimpleConsumer
import kafka.common.{OffsetMetadataAndError, TopicAndPartition}
import kafka.api.{OffsetFetchRequest, OffsetCommitRequest}


object OffsetCommitMain extends Logging {

  def main(args: Array[String]): Unit = {
    val simpleConsumer = new SimpleConsumer("localhost", 9092, 1000000, 64*1024, "test-client")

    val topic = "topic"
    // Commit an offset
    val topicAndPartition = TopicAndPartition(topic, 0)
    val commitRequest = OffsetCommitRequest("test-group", Map(topicAndPartition -> OffsetMetadataAndError(offset=42L)))
    val commitResponse = simpleConsumer.commitOffsets(commitRequest)

    System.out.println("OffsetCommitResponse: " + commitResponse.toString())

    // Fetch it and verify
    val fetchRequest = OffsetFetchRequest("test-group", Seq(topicAndPartition))
    val fetchResponse = simpleConsumer.fetchOffsets(fetchRequest)

    System.out.println("OffsetFetchResponse: " + fetchResponse.toString())


  }
}


- Jun


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


On Jan. 8, 2015, 1:54 a.m., Jun Rao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29692/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2015, 1:54 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: kafka-1841
>     https://issues.apache.org/jira/browse/kafka-1841
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> remove timestamp from version 0 protocol
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 3ee5cbad55ce836fd04bb954dcf6ef2f9bc3288f 
>   core/src/main/scala/kafka/api/OffsetCommitRequest.scala 861a6cf11dc6b6431fcbbe9de00c74a122f204bd 
>   core/src/main/scala/kafka/common/OffsetMetadataAndError.scala 4cabffeacea09a49913505db19a96a55d58c0909 
>   core/src/main/scala/kafka/server/KafkaApis.scala 2f009920af37de3cf0a3eb131f2124f4e532c4e4 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
> 
> Diff: https://reviews.apache.org/r/29692/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jun Rao
> 
>

Re: Review Request 29692: Patch for kafka-1841

Posted by Neha Narkhede <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29692/#review67149
-----------------------------------------------------------


Jun, have you had a chance to test the 0.8.1 client against the 0.8.2 server, as we had discussed earlier today?

- Neha Narkhede


On Jan. 8, 2015, 1:54 a.m., Jun Rao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29692/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2015, 1:54 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: kafka-1841
>     https://issues.apache.org/jira/browse/kafka-1841
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> remove timestamp from version 0 protocol
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a 
>   clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java 3ee5cbad55ce836fd04bb954dcf6ef2f9bc3288f 
>   core/src/main/scala/kafka/api/OffsetCommitRequest.scala 861a6cf11dc6b6431fcbbe9de00c74a122f204bd 
>   core/src/main/scala/kafka/common/OffsetMetadataAndError.scala 4cabffeacea09a49913505db19a96a55d58c0909 
>   core/src/main/scala/kafka/server/KafkaApis.scala 2f009920af37de3cf0a3eb131f2124f4e532c4e4 
>   core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala cd16ced5465d098be7a60498326b2a98c248f343 
> 
> Diff: https://reviews.apache.org/r/29692/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jun Rao
> 
>