You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jun Rao <ju...@gmail.com> on 2015/01/08 03:24:54 UTC
Re: Review Request 27391: Fix KAFKA-1634
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27391/#review67114
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
<https://reviews.apache.org/r/27391/#comment110938>
Perhaps these code can just be changed to
this(groupId, DEFAULT_GENERATION_ID, DEFAULT_CONSUMER_ID, offsetData);
clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
<https://reviews.apache.org/r/27391/#comment110939>
Same here. These code can just be replaced by forwarding the request to the next constructor.
core/src/main/scala/kafka/server/OffsetManager.scala
<https://reviews.apache.org/r/27391/#comment111015>
Shouldn't we just set the expiration time field to expirationTimestamp, instead of taking it from offsetAndMetadata?
core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala
<https://reviews.apache.org/r/27391/#comment111016>
Should we remove the commented out code?
- Jun Rao
On Dec. 2, 2014, 2:03 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27391/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2014, 2:03 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1634
> https://issues.apache.org/jira/browse/KAFKA-1634
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add another api in offset manager to return the struct, and the cache layer will only read its expiration timestamp while the offset formatter will read the struct as a whole
>
>
> 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 da29a8cb461099eb675161db2f11a9937424a5c6
> core/src/main/scala/kafka/server/KafkaApis.scala 2a1c0326b6e6966d8b8254bd6a1cb83ad98a3b80
> core/src/main/scala/kafka/server/KafkaServer.scala 1bf7d10cef23a77e716666eb16bf6d0e68bc4ebe
> core/src/main/scala/kafka/server/OffsetManager.scala 3c79428962604800983415f6f705e04f52acb8fb
> 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
>
>
Re: Review Request 27391: Fix KAFKA-1634
Posted by Guozhang Wang <wa...@gmail.com>.
> On Jan. 8, 2015, 2:24 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java, lines 79-85
> > <https://reviews.apache.org/r/27391/diff/1/?file=743505#file743505line79>
> >
> > Perhaps these code can just be changed to
> >
> > this(groupId, DEFAULT_GENERATION_ID, DEFAULT_CONSUMER_ID, offsetData);
This cannot be forwarded as in super() call we need to specify the version id.
> On Jan. 8, 2015, 2:24 a.m., Jun Rao wrote:
> > clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java, lines 97-106
> > <https://reviews.apache.org/r/27391/diff/1/?file=743505#file743505line97>
> >
> > Same here. These code can just be replaced by forwarding the request to the next constructor.
Ditto above.
> On Jan. 8, 2015, 2:24 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, lines 524-525
> > <https://reviews.apache.org/r/27391/diff/7/?file=779891#file779891line524>
> >
> > Shouldn't we just set the expiration time field to expirationTimestamp, instead of taking it from offsetAndMetadata?
For v0/1, we should just take the value of the offsetAndMetadata.timestamp, for v2 we will take the value of expirationTimestamp. This has been changed in the latest patch where offsetAndMetadata.timestamp is updated accordingly before calling offsetCommitValue().
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27391/#review67114
-----------------------------------------------------------
On Dec. 2, 2014, 2:03 a.m., Guozhang Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27391/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2014, 2:03 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1634
> https://issues.apache.org/jira/browse/KAFKA-1634
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Add another api in offset manager to return the struct, and the cache layer will only read its expiration timestamp while the offset formatter will read the struct as a whole
>
>
> 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 da29a8cb461099eb675161db2f11a9937424a5c6
> core/src/main/scala/kafka/server/KafkaApis.scala 2a1c0326b6e6966d8b8254bd6a1cb83ad98a3b80
> core/src/main/scala/kafka/server/KafkaServer.scala 1bf7d10cef23a77e716666eb16bf6d0e68bc4ebe
> core/src/main/scala/kafka/server/OffsetManager.scala 3c79428962604800983415f6f705e04f52acb8fb
> 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
>
>