> On Jan. 22, 2015, 1:56 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, lines 145-166 > > <https://reviews.apache.org/r/27391/diff/9/?file=829147#file829147line145> > > > > 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.
I think if it (the commit timestamp) is set to default value -1, we should override it according to the wiki: https://cwiki.apache.org/confluence/display/KAFKA/A+Guide+To+The+Kafka+Protocol#AGuideToTheKafkaProtocol-OffsetCommitRequest Otherwise it should not be overriden. > On Jan. 22, 2015, 1:56 a.m., Jun Rao wrote: > > core/src/main/scala/kafka/server/OffsetManager.scala, lines 121-123 > > <https://reviews.apache.org/r/27391/diff/9/?file=829150#file829150line121> > > > > 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? Changed the logic of overriding commit / expire timestamps as the following: 1. If version <= 1 or retention time is default (-1) override retention time to server default value. 2. If the original time stamp (i.e. the commit timestamp) is set to default (-1), override to the current time. 3. After 2) is done, compute the expire time to be commit timestamp + retention time. 4. Hence the above logic of checking expiration will be compatible (i.e. expiration time < now). - Guozhang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27391/#review69106 ----------------------------------------------------------- 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 > >
