----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27391/#review59839 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java <https://reviews.apache.org/r/27391/#comment101209> Would Long.MaxValue be a better default? clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java <https://reviews.apache.org/r/27391/#comment101137> I think we should mark this with @deprecated clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java <https://reviews.apache.org/r/27391/#comment101138> This can also be marked @deprecated and we can add a new constructor without timestamp and initialize to -1 there clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java <https://reviews.apache.org/r/27391/#comment101121> can you add @param for the new parameter? for consistency only - the existing javadoc is pretty pointless anyway clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java <https://reviews.apache.org/r/27391/#comment101124> If uninitialized it would be zero. However, see comment on deprecating the constructor above. core/src/main/scala/kafka/api/OffsetCommitRequest.scala <https://reviews.apache.org/r/27391/#comment101198> val retentionMs = if (versionId == 2) { ... } else { ... } core/src/main/scala/kafka/common/OffsetMetadataAndError.scala <https://reviews.apache.org/r/27391/#comment101127> Rather than making this a var it would be preferrable to use offsetAndMetadataInstance.copy(timstamp=<new val>) However, I don't think we need to modify the timestamp - see comment in KafkaApis core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/27391/#comment101166> As long as we do support v0 and v1, since we are changing the semantics of the _stored_ timestamp in the log I think it is reasonable to use the OffsetManager.retentionPeriod config here (which btw should be deprecated). i.e., if v0 or v1 request is received, pass in OffsetManager.retentionPeriod config. If it is v2 format, then just pass in retentionMs untouched. core/src/main/scala/kafka/server/OffsetManager.scala <https://reviews.apache.org/r/27391/#comment101164> Can be marked @deprecated and we can get rid of it when we get rid of v0,v1 core/src/main/scala/kafka/server/OffsetManager.scala <https://reviews.apache.org/r/27391/#comment101163> This will break - we now need to check that the absolute timestamp in the offset metadata has passed. core/src/main/scala/kafka/server/OffsetManager.scala <https://reviews.apache.org/r/27391/#comment101150> val expirationTimestamp core/src/main/scala/kafka/server/OffsetManager.scala <https://reviews.apache.org/r/27391/#comment101149> Expiration time of the offset core/src/main/scala/kafka/server/OffsetManager.scala <https://reviews.apache.org/r/27391/#comment101204> expirationTimestamp would be clearer core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala <https://reviews.apache.org/r/27391/#comment101140> Can we also enhance the OffsetCommitTest to do offset commits with v0, v1 and v2? i.e., test that the latest broker can handle all three versions correctly. - Joel Koshy On Oct. 30, 2014, 6:43 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27391/ > ----------------------------------------------------------- > > (Updated Oct. 30, 2014, 6:43 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1634 > https://issues.apache.org/jira/browse/KAFKA-1634 > > > Repository: kafka > > > Description > ------- > > The timestamp field of OffsetAndMetadata is preserved since we need to be > backward compatible with older versions > > > 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 > 050615c72efe7dbaa4634f53943bd73273d20ffb > core/src/main/scala/kafka/common/OffsetMetadataAndError.scala > 4cabffeacea09a49913505db19a96a55d58c0909 > core/src/main/scala/kafka/server/KafkaApis.scala > 968b0c4f809ea9f9c3f322aef9db105f9d2e0e23 > core/src/main/scala/kafka/server/OffsetManager.scala > 2957bc435102bc4004d8f100dbcdd56287c8ffae > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala > cd16ced5465d098be7a60498326b2a98c248f343 > > Diff: https://reviews.apache.org/r/27391/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
