kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joel Koshy" <jjko...@gmail.com>
Subject Re: Review Request 27391: Fix KAFKA-1634: Add the global retentionTime (in milis) and disable the per-offset timestamp
Date Fri, 07 Nov 2014 03:00:17 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27391/#review60285
-----------------------------------------------------------



clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
<https://reviews.apache.org/r/27391/#comment101649>

    @Deprecated



clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
<https://reviews.apache.org/r/27391/#comment101651>

    This confused me a bit, and I think it is because initCommonFields is intended to initialize
fields common to all versions of the request. It is a useful helper method but it becomes
somewhat clunky when removing fields. The partition-level timestamp is no longer a common
field.
    
    If this is v2 then we should _never_ set anything in the timestamp field of the struct;
and if it is < v2 then we should _always_ set the timestamp field (even if it is the default).
However, since the timestamp field in the Field declaration for OFFSET_COMMIT_REQUEST_PARTITION_V0
does not have a default explicitly specified, I think this will break with a SchemaException("missing
value...") for offset commit request v0, v1 if we choose to write to a bytebuffer under those
versions with this code.
    
    One option is to explicitly pass in the constructor version (0, 1, 2) to initCommonFields
and use that to decide whether to include/exclude this field, but that is weird. Another alternative
is a separate helper method for v0v1. That is even weirder.



clients/src/main/java/org/apache/kafka/common/requests/OffsetCommitRequest.java
<https://reviews.apache.org/r/27391/#comment101650>

    Would help to add a comment "This field only exists in v0 and v1"



core/src/main/scala/kafka/common/OffsetMetadataAndError.scala
<https://reviews.apache.org/r/27391/#comment101657>

    Can we mark this @Deprecated as well?
    
    We should probably make the primary constructor without timestamp and add a secondary
constructor with timestamp and mark deprecated there.
    
    Also, can we use case class.copy if timestamp needs to be modified? However, per comment
further down I don't think it needs to be touched.



core/src/main/scala/kafka/server/OffsetManager.scala
<https://reviews.apache.org/r/27391/#comment101658>

    This was already there, but it would be clearer to use:
    
    filter { case (..., ...) => 
    }



core/src/main/scala/kafka/server/OffsetManager.scala
<https://reviews.apache.org/r/27391/#comment101659>

    Found %d expired offsets.



core/src/main/scala/kafka/server/OffsetManager.scala
<https://reviews.apache.org/r/27391/#comment101660>

    Actually, since we always set the retention period (for v0, v1) in KafkaApis do we need
to even touch this timestamp? i.e., we should basically ignore it right? So we only need to
do:
    value.set(VALUE_TIMESTAMP_FIELD, expirationTimestamp).


- Joel Koshy


On Nov. 6, 2014, 11:35 p.m., Guozhang Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27391/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2014, 11:35 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

>   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/common/OffsetMetadataAndError.scala 4cabffeacea09a49913505db19a96a55d58c0909

>   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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message