Uploaded image for project: 'Kafka'
  1. Kafka
  2. KAFKA-1841

OffsetCommitRequest API - timestamp field is not versioned

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.8.2.0
    • Fix Version/s: 0.8.2.0
    • Component/s: core
    • Labels:
      None
    • Environment:
      wire-protocol

      Description

      Timestamp field was added to the OffsetCommitRequest wire protocol api for 0.8.2 by KAFKA-1012 . The 0.8.1.1 server does not support the timestamp field, so I think the api version of OffsetCommitRequest should be incremented and checked by the 0.8.2 kafka server before attempting to read a timestamp from the network buffer in OffsetCommitRequest.readFrom (core/src/main/scala/kafka/api/OffsetCommitRequest.scala)

      It looks like a subsequent patch (KAFKA-1462) added another api change to support a new constructor w/ params generationId and consumerId, calling that version 1, and a pending patch (KAFKA-1634) adds retentionMs as another field, while possibly removing timestamp altogether, calling this version 2. So the fix here is not straightforward enough for me to submit a patch.

      This could possibly be merged into KAFKA-1634, but opening as a separate Issue because I believe the lack of versioning in the current trunk should block 0.8.2 release.

      1. kafka-1841_2015-01-08_15:07:57.patch
        25 kB
        Jun Rao
      2. kafka-1841_2015-01-09_14:36:50.patch
        24 kB
        Jun Rao
      3. kafka-1841_2015-01-12_14:30:24.patch
        29 kB
        Jun Rao
      4. kafka-1841.patch
        14 kB
        Jun Rao

        Issue Links

          Activity

          Hide
          joestein Joe Stein added a comment -

          In addition to the issue you bring up, the functionality as a whole has changed.. when you call OffsetFetchRequest the version = 0 needs to preserve the old functionality https://github.com/apache/kafka/blob/0.8.1/core/src/main/scala/kafka/server/KafkaApis.scala#L678-L700 and version = 1 the new https://github.com/apache/kafka/blob/0.8.2/core/src/main/scala/kafka/server/KafkaApis.scala#L153-L223. Also the OffsetFetchRequest functionality even though the wire protocol is the same after the 0.8.2 upgrade for OffsetFetchRequest if you were using 0.8.1.1 OffsetFetchRequest https://github.com/apache/kafka/blob/0.8.1/core/src/main/scala/kafka/server/KafkaApis.scala#L705-L728 will stop going to zookeeper and start going to Kafka storage https://github.com/apache/kafka/blob/0.8.2/core/src/main/scala/kafka/server/KafkaApis.scala#L504-L519 so more errors will happen and things break too.

          Show
          joestein Joe Stein added a comment - In addition to the issue you bring up, the functionality as a whole has changed.. when you call OffsetFetchRequest the version = 0 needs to preserve the old functionality https://github.com/apache/kafka/blob/0.8.1/core/src/main/scala/kafka/server/KafkaApis.scala#L678-L700 and version = 1 the new https://github.com/apache/kafka/blob/0.8.2/core/src/main/scala/kafka/server/KafkaApis.scala#L153-L223 . Also the OffsetFetchRequest functionality even though the wire protocol is the same after the 0.8.2 upgrade for OffsetFetchRequest if you were using 0.8.1.1 OffsetFetchRequest https://github.com/apache/kafka/blob/0.8.1/core/src/main/scala/kafka/server/KafkaApis.scala#L705-L728 will stop going to zookeeper and start going to Kafka storage https://github.com/apache/kafka/blob/0.8.2/core/src/main/scala/kafka/server/KafkaApis.scala#L504-L519 so more errors will happen and things break too.
          Hide
          junrao Jun Rao added a comment -

          Thinking about this a bit more. We can revert the wire format of version 0 to what's in 0.8.1. However, I don't think that we need to preserve the behavior of writing to ZK though. Kafka managed offset will be the future since we want to reduce the load on ZK. The offset topic should be highly available. If there is any error due to bugs, we should just fix those.

          Show
          junrao Jun Rao added a comment - Thinking about this a bit more. We can revert the wire format of version 0 to what's in 0.8.1. However, I don't think that we need to preserve the behavior of writing to ZK though. Kafka managed offset will be the future since we want to reduce the load on ZK. The offset topic should be highly available. If there is any error due to bugs, we should just fix those.
          Hide
          parth.brahmbhatt Parth Brahmbhatt added a comment -

          @junrao If noone plans to take this one, please assign to me.

          Show
          parth.brahmbhatt Parth Brahmbhatt added a comment - @junrao If noone plans to take this one, please assign to me.
          Hide
          junrao Jun Rao added a comment -

          Parth,

          Thanks for offering. I have a patch that's almost ready. Will upload soon.

          Show
          junrao Jun Rao added a comment - Parth, Thanks for offering. I have a patch that's almost ready. Will upload soon.
          Hide
          junrao Jun Rao added a comment -

          Created reviewboard https://reviews.apache.org/r/29692/diff/
          against branch origin/0.8.2

          Show
          junrao Jun Rao added a comment - Created reviewboard https://reviews.apache.org/r/29692/diff/ against branch origin/0.8.2
          Hide
          junrao Jun Rao added a comment -

          Dana Powers, could you try this patch on 0.8.2 and see if it works with the 0.8.1 client? The implementation on the server uses a special Kafka topic, instead of ZK. You may see a transient error when the special topic is created for the first time.

          Show
          junrao Jun Rao added a comment - Dana Powers , could you try this patch on 0.8.2 and see if it works with the 0.8.1 client? The implementation on the server uses a special Kafka topic, instead of ZK. You may see a transient error when the special topic is created for the first time.
          Hide
          junrao Jun Rao added a comment -

          Updated reviewboard https://reviews.apache.org/r/29692/diff/
          against branch origin/0.8.2

          Show
          junrao Jun Rao added a comment - Updated reviewboard https://reviews.apache.org/r/29692/diff/ against branch origin/0.8.2
          Hide
          junrao Jun Rao added a comment -

          This is actually a bit tricky to fix. To make this really backward compatible, we have to make sure that version 0 of OffsetCommitRequest only writes to ZK. However, this doesn't quite work together with OffsetFetchRequest since in 0.8.2, it only has one version and it always reads offsets from Kafka. To address this issue, I bumped up the version of OffsetFetchRequest in 0.8.2 (with same wire protocol). Then, version 0 of OffsetFetchRequest will read from ZK and version 1 of OffsetFetchRequest will read from Kafka. This works as long as people are only using released final version. However, since this introduces an incompatible change of OffsetFetchRequest in 0.8.2-beta and trunk, this will create problems for people (assuming that they are using this api) who have a deployment of 0.8.2-beta and want to upgrade to 0.8.2 final, or a deployment from trunk and want to upgrade to a later version of trunk in the future. In either case, the upgrade of the broker will cause the old client to behave differently and incorrectly.

          Show
          junrao Jun Rao added a comment - This is actually a bit tricky to fix. To make this really backward compatible, we have to make sure that version 0 of OffsetCommitRequest only writes to ZK. However, this doesn't quite work together with OffsetFetchRequest since in 0.8.2, it only has one version and it always reads offsets from Kafka. To address this issue, I bumped up the version of OffsetFetchRequest in 0.8.2 (with same wire protocol). Then, version 0 of OffsetFetchRequest will read from ZK and version 1 of OffsetFetchRequest will read from Kafka. This works as long as people are only using released final version. However, since this introduces an incompatible change of OffsetFetchRequest in 0.8.2-beta and trunk, this will create problems for people (assuming that they are using this api) who have a deployment of 0.8.2-beta and want to upgrade to 0.8.2 final, or a deployment from trunk and want to upgrade to a later version of trunk in the future. In either case, the upgrade of the broker will cause the old client to behave differently and incorrectly.
          Hide
          junrao Jun Rao added a comment -

          Updated reviewboard https://reviews.apache.org/r/29692/diff/
          against branch origin/0.8.2

          Show
          junrao Jun Rao added a comment - Updated reviewboard https://reviews.apache.org/r/29692/diff/ against branch origin/0.8.2
          Hide
          junrao Jun Rao added a comment -

          Updated reviewboard https://reviews.apache.org/r/29692/diff/
          against branch origin/0.8.2

          Show
          junrao Jun Rao added a comment - Updated reviewboard https://reviews.apache.org/r/29692/diff/ against branch origin/0.8.2
          Hide
          junrao Jun Rao added a comment -

          Thanks for the review. Committed to 0.8.2.

          Since we are evolving the protocol of OffsetCommitRequest in KAFKA-1634, will let KAFKA-1634 merge in the changes in this jira to trunk.

          Show
          junrao Jun Rao added a comment - Thanks for the review. Committed to 0.8.2. Since we are evolving the protocol of OffsetCommitRequest in KAFKA-1634 , will let KAFKA-1634 merge in the changes in this jira to trunk.

            People

            • Assignee:
              junrao Jun Rao
              Reporter:
              dana.powers Dana Powers
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development