----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27391/#review61761 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java <https://reviews.apache.org/r/27391/#comment103640> Not sure if this will make it worse; would it be clearer to call this DEFAULT_TIMESTAMP_V0_V1? clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java <https://reviews.apache.org/r/27391/#comment103719> Made a follow-up comment in the earlier RB. But pasting here as well: Agree that it is still "common" in the object but it is completely removed from the wire protocol - i.e., OFFSET_COMMIT_REQUEST_PARTITION_V1 which is in the V2 OffsetCommitRequest does not have a timestamp. This method should probably be read as "initCommonFieldsInStruct" - i.e., effectively the wire protocol. That said, I'm loathe to add another init method which reads initCommonFieldsInV0AndV1. So I think rather than checking fetchPartitionData.timestamp it would be better to explicitly check the (already set) request version in the struct. If v0 or v1 then set the timestamp key name. core/src/main/scala/kafka/api/OffsetCommitRequest.scala <https://reviews.apache.org/r/27391/#comment103727> >= 2 (or we may forget when we go to version 3) core/src/main/scala/kafka/common/OffsetMetadataAndError.scala <https://reviews.apache.org/r/27391/#comment103734> I think our coding convention is to use the parameterless form in the absence of side-effects core/src/main/scala/kafka/common/OffsetMetadataAndError.scala <https://reviews.apache.org/r/27391/#comment103735> Similar comment as above. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/27391/#comment103737> See reply to earlier comment. - Joel Koshy On Nov. 8, 2014, 12:54 a.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27391/ > ----------------------------------------------------------- > > (Updated Nov. 8, 2014, 12:54 a.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 > > 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 > fbc680fde21b02f11285a4f4b442987356abd17b > core/src/main/scala/kafka/server/KafkaApis.scala > 968b0c4f809ea9f9c3f322aef9db105f9d2e0e23 > core/src/main/scala/kafka/server/KafkaServer.scala > 4de812374e8fb1fed834d2be3f9655f55b511a74 > core/src/main/scala/kafka/server/OffsetManager.scala > 2957bc435102bc4004d8f100dbcdd56287c8ffae > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala > cd16ced5465d098be7a60498326b2a98c248f343 > core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala > 8c5364fa97da1be09973c176d1baeb339455d319 > > Diff: https://reviews.apache.org/r/27391/diff/ > > > Testing > ------- > > > Thanks, > > Guozhang Wang > >
