----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27391/#review77970 -----------------------------------------------------------
Ship it! Thank you for the updated patch. Just one minor comment below on suggested comments to add. Otherwise I think this looks good! core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/27391/#comment126341> may be worth adding comments to describe these scenarios: ``` // - Commit timestamp is always set to now. // - "Default" expiration timestamp is now + retention (and retention may be overridden if v2) // - Expire timestamp is computed differently for v1 and v2. // - If v1 and no explicit commit timestamp is provided we use default expiration timestamp. // - If v1 and explicit commit timestamp is provided we calculate retention from that explicit commit timestamp // - If v2 we use the default expiration timestamp ``` - Joel Koshy On March 26, 2015, 7:28 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27391/ > ----------------------------------------------------------- > > (Updated March 26, 2015, 7:28 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1634 > https://issues.apache.org/jira/browse/KAFKA-1634 > > > Repository: kafka > > > Description > ------- > > 1. Always override commit time to now, only change expire time accorinding to > the commit time. > 2. Override expire time upon loading offsets of version 1. > 3. Change new consumer to use the new version 2. > > > Diffs > ----- > > > clients/src/main/java/org/apache/kafka/clients/consumer/internals/Coordinator.java > 436f9b2a843bc8c44d17403f5880b6736a5d56a8 > clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java > 101f382170ad6740b3f8ff2d27b93a64874a857f > clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java > 7672a3a0d674d101078651956d7122059e59e6d5 > > clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java > 94e9d376235b3288836807d8e8d2547b3743aad5 > > clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java > 13237fd72da5448a3d596b882fef141f336f827d > 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 > 1584a92447d276b6206d212b0b5487c352044154 > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala > cca815a128419e146feff53adaeddc901bb5de1f > core/src/main/scala/kafka/server/KafkaApis.scala > 35af98f0bc1b6a50bd1d97a30147593f8c6a422d > core/src/main/scala/kafka/server/KafkaConfig.scala > 46d21c73f1feb3410751899380b35da0c37c975c > core/src/main/scala/kafka/server/KafkaServer.scala > dddef938fabae157ed8644536eb1a2f329fb42b7 > core/src/main/scala/kafka/server/OffsetManager.scala > d05e14d2018c0a0b5d22697313e9abde4363d65d > core/src/main/scala/kafka/server/ReplicaManager.scala > c5274822c57bf3c1f9e4135c0bdcaa87ee50ce20 > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala > fba852afa1b2f46b61e2fd12c38c821ba04e9cc6 > core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala > e4d0435eb4213597c2fb9c3f2093c227de53a417 > core/src/test/scala/unit/kafka/server/ServerShutdownTest.scala > b46daa436231d5aa5c1e2992fd5c2d9a73a30c80 > > Diff: https://reviews.apache.org/r/27391/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
