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