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

Improve semantics of timestamp in OffsetCommitRequests and update documentation

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.9.0.0
    • Component/s: None
    • Labels:
      None

      Description

      From the mailing list -

      following up on this – I think the online API docs for OffsetCommitRequest
      still incorrectly refer to client-side timestamps:

      https://cwiki.apache.org/confluence/display/KAFKA/A+Guide+To+The+Kafka+Protocol#AGuideToTheKafkaProtocol-OffsetCommitRequest

      Wasn't that removed and now always handled server-side now? Would one of
      the devs mind updating the API spec wiki?

      1. KAFKA-1634.patch
        33 kB
        Guozhang Wang
      2. KAFKA-1634_2014-11-06_15:35:46.patch
        51 kB
        Guozhang Wang
      3. KAFKA-1634_2014-11-07_16:54:33.patch
        63 kB
        Guozhang Wang
      4. KAFKA-1634_2014-11-17_17:42:44.patch
        68 kB
        Guozhang Wang
      5. KAFKA-1634_2014-11-21_14:00:34.patch
        71 kB
        Guozhang Wang
      6. KAFKA-1634_2014-12-01_11:44:35.patch
        79 kB
        Guozhang Wang
      7. KAFKA-1634_2014-12-01_18:03:12.patch
        94 kB
        Guozhang Wang
      8. KAFKA-1634_2015-01-14_15:50:15.patch
        123 kB
        Guozhang Wang
      9. KAFKA-1634_2015-01-21_16:43:01.patch
        127 kB
        Guozhang Wang
      10. KAFKA-1634_2015-01-22_18:47:37.patch
        136 kB
        Guozhang Wang
      11. KAFKA-1634_2015-01-23_16:06:07.patch
        140 kB
        Guozhang Wang
      12. KAFKA-1634_2015-02-06_11:01:08.patch
        168 kB
        Guozhang Wang
      13. KAFKA-1634_2015-03-26_12:16:09.patch
        192 kB
        Guozhang Wang
      14. KAFKA-1634_2015-03-26_12:27:18.patch
        194 kB
        Guozhang Wang

        Issue Links

          Activity

          Hide
          junrao Jun Rao added a comment -

          The wiki is actually consistent with the code in trunk. The timestamp is used to decide whether some old consumer offsets can be garbage collected. Typically, the timestamp should just be the time when the offset is committed (and can be obtained just on the broker side). However, this idea is that a client, if needed, can set a larger timestamp if it wants to preserve the offset for longer. It's not clear to me if this is super useful. We can debate that in a separate jira if needed.

          Show
          junrao Jun Rao added a comment - The wiki is actually consistent with the code in trunk. The timestamp is used to decide whether some old consumer offsets can be garbage collected. Typically, the timestamp should just be the time when the offset is committed (and can be obtained just on the broker side). However, this idea is that a client, if needed, can set a larger timestamp if it wants to preserve the offset for longer. It's not clear to me if this is super useful. We can debate that in a separate jira if needed.
          Hide
          nehanarkhede Neha Narkhede added a comment -

          Jun Rao, that explanation is actually pretty unintuitive. So seems like this is something that needs to be fixed. The client shouldn't need to specify a timestamp for an offset commit. Unless I'm missing something that it is designed for. Joel Koshy, any idea?

          Show
          nehanarkhede Neha Narkhede added a comment - Jun Rao , that explanation is actually pretty unintuitive. So seems like this is something that needs to be fixed. The client shouldn't need to specify a timestamp for an offset commit. Unless I'm missing something that it is designed for. Joel Koshy , any idea?
          Hide
          nehanarkhede Neha Narkhede added a comment -

          Reopening until we have filed a JIRA to fix the issue.

          Show
          nehanarkhede Neha Narkhede added a comment - Reopening until we have filed a JIRA to fix the issue.
          Hide
          junrao Jun Rao added a comment -

          One use case is that if a client never wants the offset to be garbage collected on the broker, it can set timestamp to be max long.

          Show
          junrao Jun Rao added a comment - One use case is that if a client never wants the offset to be garbage collected on the broker, it can set timestamp to be max long.
          Hide
          nehanarkhede Neha Narkhede added a comment -

          How general is that use case and if it just a means for picking infinite retention, then why not just have a boolean option rather than a full timestamp? My main concern is that this is looking more like an after thought instead of a well thought out API. We should think how the user would interpret the timestamp and what makes sense as a general solution.

          Show
          nehanarkhede Neha Narkhede added a comment - How general is that use case and if it just a means for picking infinite retention, then why not just have a boolean option rather than a full timestamp? My main concern is that this is looking more like an after thought instead of a well thought out API. We should think how the user would interpret the timestamp and what makes sense as a general solution.
          Hide
          junrao Jun Rao added a comment -

          Another use case is the following. It may not make sense to retain an offset longer than the data retention time. So, if data is kept for only 7 days, a consumer client may want to set timestamp to be now + 7 days to preserve the offset as long as it's still valid.

          Joel probably can comment on the general use cases more.

          Show
          junrao Jun Rao added a comment - Another use case is the following. It may not make sense to retain an offset longer than the data retention time. So, if data is kept for only 7 days, a consumer client may want to set timestamp to be now + 7 days to preserve the offset as long as it's still valid. Joel probably can comment on the general use cases more.
          Hide
          nehanarkhede Neha Narkhede added a comment -

          Basically, I'm not sure this is great user experience. When I looked at the API, I found it awkward to use and understand why I needed to specify a timestamp. If there are use cases that are general, I'd like to hear and maybe we should update documentation. But if not, we should remove it. Joel Koshy Any comments?

          Show
          nehanarkhede Neha Narkhede added a comment - Basically, I'm not sure this is great user experience. When I looked at the API, I found it awkward to use and understand why I needed to specify a timestamp. If there are use cases that are general, I'd like to hear and maybe we should update documentation. But if not, we should remove it. Joel Koshy Any comments?
          Hide
          jjkoshy Joel Koshy added a comment -

          Neha Narkhede I agree that timestamp in the request API is a bit odd since that is an implementation detail on the server-side (on how long to retain offsets) and setting "future" timestamps on the client-side is a hack to "trick" the server into retaining the offset.

          The use-cases though are as Jun described - i.e., if a client wants to ensure that offsets are retained for a certain period of time (regardless of when the server cleans up stale offsets). That said, the field could be renamed and reimplemented as a retentionPeriod field (not timestamp). i.e., if set the consumer would explicitly say "retain my offsets for X milliseconds/days/whatever". The broker can have a second hard staleness threshold to cap this client-driven staleness threshold.

          Show
          jjkoshy Joel Koshy added a comment - Neha Narkhede I agree that timestamp in the request API is a bit odd since that is an implementation detail on the server-side (on how long to retain offsets) and setting "future" timestamps on the client-side is a hack to "trick" the server into retaining the offset. The use-cases though are as Jun described - i.e., if a client wants to ensure that offsets are retained for a certain period of time (regardless of when the server cleans up stale offsets). That said, the field could be renamed and reimplemented as a retentionPeriod field (not timestamp). i.e., if set the consumer would explicitly say "retain my offsets for X milliseconds/days/whatever". The broker can have a second hard staleness threshold to cap this client-driven staleness threshold.
          Hide
          nehanarkhede Neha Narkhede added a comment -

          Joel Koshy Thanks for the explanation. If retention is the use case, it is easier to understand if it was called retentionPeriod. Since 0.8.2 is when this releases, it will be great to make that renaming change before 0.8.2 goes out. If there is a broker side feature that is required for this to work properly and it's not done, then we can either get rid of this and add it when it's useful or leave it in there as a no-op or with a warning. What do you prefer?

          Show
          nehanarkhede Neha Narkhede added a comment - Joel Koshy Thanks for the explanation. If retention is the use case, it is easier to understand if it was called retentionPeriod. Since 0.8.2 is when this releases, it will be great to make that renaming change before 0.8.2 goes out. If there is a broker side feature that is required for this to work properly and it's not done, then we can either get rid of this and add it when it's useful or leave it in there as a no-op or with a warning. What do you prefer?
          Hide
          jjkoshy Joel Koshy added a comment -

          Actually, one more potential source of confusion is that we use OffsetAndMetadata for both offset commits requests and offset fetch responses.

          i.e., an OffsetFetchResponse will contain: offset, metadata and this timestamp field. The timestamp field should really be ignored. It is annoying to document such things - i.e., tell users to just ignore the field.

          Ideally, I think we should do the following:

          • Remove the timestamp from the OffsetAndMetadata class
          • Move it to the top-level of the OffsetCommitRequest and rename it to retentionMs
          • The broker will compute the absolute time (based off time of receipt) that the offset should be expired
          • The above absolute time will continue to be stored in the offsets topic and the cleanup thread can remove those offsets when they are past their TTL.
          • OffsetFetchResponse will just return OffsetAndMetadata (no timestamp)

          We (linkedin and possibly others) already deployed this to some of our consumers but if we can bump up the protocol version when doing the above and translate requests that come in with the older version I think it should be okay.

          Show
          jjkoshy Joel Koshy added a comment - Actually, one more potential source of confusion is that we use OffsetAndMetadata for both offset commits requests and offset fetch responses. i.e., an OffsetFetchResponse will contain: offset, metadata and this timestamp field. The timestamp field should really be ignored. It is annoying to document such things - i.e., tell users to just ignore the field. Ideally, I think we should do the following: Remove the timestamp from the OffsetAndMetadata class Move it to the top-level of the OffsetCommitRequest and rename it to retentionMs The broker will compute the absolute time (based off time of receipt) that the offset should be expired The above absolute time will continue to be stored in the offsets topic and the cleanup thread can remove those offsets when they are past their TTL. OffsetFetchResponse will just return OffsetAndMetadata (no timestamp) We (linkedin and possibly others) already deployed this to some of our consumers but if we can bump up the protocol version when doing the above and translate requests that come in with the older version I think it should be okay.
          Hide
          jjkoshy Joel Koshy added a comment -

          (BTW, I'm looking for a +1 or -1 on the above comment )

          Show
          jjkoshy Joel Koshy added a comment - (BTW, I'm looking for a +1 or -1 on the above comment )
          Hide
          junrao Jun Rao added a comment -

          Joel,

          I don't see OffsetFetchResponse contain timestamp (it only contains offset, metadata and error code). I agree that it's better to move retentionMs to the topic-level of OffsetCommitRequest. We can bump up the protocol version if needed.

          Show
          junrao Jun Rao added a comment - Joel, I don't see OffsetFetchResponse contain timestamp (it only contains offset, metadata and error code). I agree that it's better to move retentionMs to the topic-level of OffsetCommitRequest. We can bump up the protocol version if needed.
          Hide
          nehanarkhede Neha Narkhede added a comment -

          +1 to your suggestions above. Thanks Joel Koshy

          Show
          nehanarkhede Neha Narkhede added a comment - +1 to your suggestions above. Thanks Joel Koshy
          Hide
          jjkoshy Joel Koshy added a comment -

          Jun Rao yes you are right. The OffsetFetchResponse returns offset, metadata and error - it does not include the timestamp.

          Show
          jjkoshy Joel Koshy added a comment - Jun Rao yes you are right. The OffsetFetchResponse returns offset, metadata and error - it does not include the timestamp.
          Hide
          guozhang Guozhang Wang added a comment -

          I will work on this after KAFKA-1583.

          Show
          guozhang Guozhang Wang added a comment - I will work on this after KAFKA-1583 .
          Hide
          junrao Jun Rao added a comment -

          Moving this to 0.8.3 since it's easier to fix after KAFKA-1583 is done.

          Show
          junrao Jun Rao added a comment - Moving this to 0.8.3 since it's easier to fix after KAFKA-1583 is done.
          Hide
          guozhang Guozhang Wang added a comment -

          While working on this: should a retention time better be scaled at secs than ms? Do we have scenarios where people want to just retain their offsets for miliseconds? If not then we may end up always having very large number for this field.

          Show
          guozhang Guozhang Wang added a comment - While working on this: should a retention time better be scaled at secs than ms? Do we have scenarios where people want to just retain their offsets for miliseconds? If not then we may end up always having very large number for this field.
          Hide
          guozhang Guozhang Wang added a comment -

          Created reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Created reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          dana.powers Dana Powers added a comment -

          possibly related to this JIRA: KAFKA-1841 . The timestamp field itself was not in the released api version 0 and if it is to be included in 0.8.2 (this JIRA suggests it is, but to be removed in 0.8.3 ?) then I think it will need to be versioned.

          Show
          dana.powers Dana Powers added a comment - possibly related to this JIRA: KAFKA-1841 . The timestamp field itself was not in the released api version 0 and if it is to be included in 0.8.2 (this JIRA suggests it is, but to be removed in 0.8.3 ?) then I think it will need to be versioned.
          Hide
          junrao Jun Rao added a comment -

          Committed KAFKA-1841 to 0.8.2. It would be easier to incorporate the changes in KAFKA-1841 into this jira and commit them together to trunk.

          Show
          junrao Jun Rao added a comment - Committed KAFKA-1841 to 0.8.2. It would be easier to incorporate the changes in KAFKA-1841 into this jira and commit them together to trunk.
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment - - edited

          Joel Koshy, Jun Rao My plan is to check in this patch first and than rebase 1481 on that, so could you take a look at the latest patch incorporating your comments?

          Show
          guozhang Guozhang Wang added a comment - - edited Joel Koshy , Jun Rao My plan is to check in this patch first and than rebase 1481 on that, so could you take a look at the latest patch incorporating your comments?
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          junrao Jun Rao added a comment -

          I guess you mean rebasing KAFAK-1841, instead of KAFKA-1481?

          Show
          junrao Jun Rao added a comment - I guess you mean rebasing KAFAK-1841, instead of KAFKA-1481 ?
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          junrao Jun Rao added a comment -

          Guozhang Wang, Joel Koshy, in order to work on KAFKA-1927, we will need to have KAFKA-1841 merged into trunk, which depends on this jira.

          Show
          junrao Jun Rao added a comment - Guozhang Wang , Joel Koshy , in order to work on KAFKA-1927 , we will need to have KAFKA-1841 merged into trunk, which depends on this jira.
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment -

          Updated reviewboard https://reviews.apache.org/r/27391/diff/
          against branch origin/trunk

          Show
          guozhang Guozhang Wang added a comment - Updated reviewboard https://reviews.apache.org/r/27391/diff/ against branch origin/trunk
          Hide
          guozhang Guozhang Wang added a comment -

          Thanks for the reviews, committed to trunk.

          Show
          guozhang Guozhang Wang added a comment - Thanks for the reviews, committed to trunk.
          Hide
          jjkoshy Joel Koshy added a comment -

          Guozhang Wang we actually need to merge KAFKA-1841 now. Or do you want to do that in a separate jira?

          Show
          jjkoshy Joel Koshy added a comment - Guozhang Wang we actually need to merge KAFKA-1841 now. Or do you want to do that in a separate jira?
          Hide
          junrao Jun Rao added a comment -

          Also, could you also update the protocol wiki (https://cwiki.apache.org/confluence/display/KAFKA/A+Guide+To+The+Kafka+Protocol#AGuideToTheKafkaProtocol-OffsetCommitRequest)? For the new version, please document the release from which the new version will be supported.

          Show
          junrao Jun Rao added a comment - Also, could you also update the protocol wiki ( https://cwiki.apache.org/confluence/display/KAFKA/A+Guide+To+The+Kafka+Protocol#AGuideToTheKafkaProtocol-OffsetCommitRequest)? For the new version, please document the release from which the new version will be supported.
          Hide
          guozhang Guozhang Wang added a comment -

          Just updated the protocol wiki with the new version. As for merging KAFKA-1841, I can do that after 04/06; if that is too late some one can pick it up.

          Show
          guozhang Guozhang Wang added a comment - Just updated the protocol wiki with the new version. As for merging KAFKA-1841 , I can do that after 04/06; if that is too late some one can pick it up.
          Hide
          jjkoshy Joel Koshy added a comment -

          Reopening since we need to merge KAFKA-1841

          Show
          jjkoshy Joel Koshy added a comment - Reopening since we need to merge KAFKA-1841
          Hide
          junrao Jun Rao added a comment -

          Perhaps, we can do KAFKA-1841 together with KAFKA-2068. There will probably be less duplicated code that way.

          Show
          junrao Jun Rao added a comment - Perhaps, we can do KAFKA-1841 together with KAFKA-2068 . There will probably be less duplicated code that way.
          Hide
          jjkoshy Joel Koshy added a comment -

          Yes I think that makes more sense

          Show
          jjkoshy Joel Koshy added a comment - Yes I think that makes more sense

            People

            • Assignee:
              guozhang Guozhang Wang
              Reporter:
              nehanarkhede Neha Narkhede
              Reviewer:
              Joel Koshy
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development