----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27391/#review69106 -----------------------------------------------------------
Thanks for the patch. A few more comments. clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java <https://reviews.apache.org/r/27391/#comment113727> Would it be better to use -1L as the default retention time? MAX_VALUE could be useful for the case when a client wants the offset never to be expired. core/src/main/scala/kafka/api/OffsetCommitRequest.scala <https://reviews.apache.org/r/27391/#comment113724> It seems that our coding convention has been not to use {} on a single line in the body. So, we use if () do sth instead of if () { do sth } core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/27391/#comment113730> I am not sure that we should change the timestamp for offsets produced in V0 and V1. There could be data in the offset topic already written by 0.8.2 code. See the other comment in OffsetManager on expiring. core/src/main/scala/kafka/server/OffsetManager.scala <https://reviews.apache.org/r/27391/#comment113729> Does that change work correctly with offsets already stored in v0 and v1 format using 0.8.2 code? Would those offsets still be expired at the right time? - Jun Rao On Jan. 22, 2015, 12:43 a.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27391/ > ----------------------------------------------------------- > > (Updated Jan. 22, 2015, 12:43 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1634 > https://issues.apache.org/jira/browse/KAFKA-1634 > > > Repository: kafka > > > Description > ------- > > Incorporated Joel's comments > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java > 7517b879866fc5dad5f8d8ad30636da8bbe7784a > clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java > 121e880a941fcd3e6392859edba11a94236494cc > > clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java > 3ee5cbad55ce836fd04bb954dcf6ef2f9bc3288f > > clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java > df37fc6d8f0db0b8192a948426af603be3444da4 > core/src/main/scala/kafka/api/OffsetCommitRequest.scala > 050615c72efe7dbaa4634f53943bd73273d20ffb > core/src/main/scala/kafka/api/OffsetFetchRequest.scala > c7604b9cdeb8f46507f04ed7d35fcb3d5f150713 > core/src/main/scala/kafka/common/OffsetMetadataAndError.scala > 4cabffeacea09a49913505db19a96a55d58c0909 > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala > 191a8677444e53b043e9ad6e94c5a9191c32599e > core/src/main/scala/kafka/server/KafkaApis.scala > ec8d9f7ba44741db40875458bd524c4062ad6a26 > core/src/main/scala/kafka/server/KafkaConfig.scala > 6d74983472249eac808d361344c58cc2858ec971 > core/src/main/scala/kafka/server/KafkaServer.scala > 89200da30a04943f0b9befe84ab17e62b747c8c4 > core/src/main/scala/kafka/server/OffsetManager.scala > 0bdd42fea931cddd072c0fff765b10526db6840a > core/src/main/scala/kafka/server/ReplicaManager.scala > e58fbb922e93b0c31dff04f187fcadb4ece986d7 > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala > cd16ced5465d098be7a60498326b2a98c248f343 > core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala > 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5 > core/src/test/scala/unit/kafka/server/ServerShutdownTest.scala > ba1e48e4300c9fb32e36e7266cb05294f2a481e5 > > Diff: https://reviews.apache.org/r/27391/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
