Hi. Guozhang,
For fetch/produce requests, even though there is no protocol change, it is
a semantic change if the timestamp can be negative. Some client
implementations may depend on that. So, it's probably better to bump up the
version of those requests.
Thanks,
Jun
On Sun, Dec 9, 2018 at 9:21 PM Guozhang Wang <wangg...@gmail.com> wrote:
> Hi folks,
>
> Thanks for your replies! Just to clarify, the proposal itself does not
> introduce any additional fields in the message format (some new attributes
> are mentioned in the Rejected Alternatives though), so my understand is
> that we do not need to increment the magic byte version of the message
> itself.
>
> Also, ConsumerRecord.NO_TIMESTAMP (-1) is still used as "no timestamp", and
> as we've discussed before it is a good trade-off to say "we do not have way
> express Wednesday, December 31, 1969 11:59:59.999". I.e. we are extending
> the timestamp to have other negative values than -1, but we did not change
> the semantics if value -1 itself. So I think although we do bump up the
> request protocol version for semantics changes, it is not necessary for
> this case (please correct me if there are cases that would not work for
> it).
>
> I do agree with Magnus's point that for ListOffsetsRequest, we should
> consider using different values than -1 / -2 to indicate `EARLEST / LATEST
> TIMESTAMP` now since for example timestamp -2 does have a meaningful
> semantics now, and hence its protocol version would need bump as we change
> its field. And I think a single byte indicating the type as (EARLEST,
> LATEST, ACTUAL_TIMESTAMP_VALUE) should be sufficient, but I'll leave it to
> Konstandin to decide if he wants to do this KIP or do it in another
> follow-up KIP.
>
>
> Guozhang
>
>
> On Fri, Dec 7, 2018 at 5:06 PM Jun Rao <j...@confluent.io> wrote:
>
> > Hi, Konstandin,
> >
> > Thanks for the KIP. I agree with Magnus on the protocol version changes.
> As
> > for the sentinel value, currently ConsumerRecord.NO_TIMESTAMP (-1) is
> used
> > for V0 message format. For compatibility, it seems that we still need to
> > preserve that.
> >
> > Jun
> >
> > On Thu, Dec 6, 2018 at 2:32 AM Magnus Edenhill <mag...@edenhill.se>
> wrote:
> >
> > > Sorry for getting in the game this late, and on the wrong thread!
> > >
> > > I think negative timestamps makes sense and is a good addition,
> > > but I have a couple of concerns with the proposal:
> > >
> > > 1. I believe any change to the protocol format or semantics require a
> > > protocol bump, in this case for ProduceRequest, FetchRequest,
> > > ListOffsetsRequest.
> > > 2. ListOffsetsRequest should be changed to allow logical (END,
> > BEGINNING)
> > > and absolute lookups without special treatment of two absolute values
> as
> > > logical (-1, -2), this seems like a hack and will require application
> > logic
> > > to avoid these timestamps, that's leaky abstraction.
> > > Perhaps add a new field `int8 LookupType = { BEGINNING=-2, END=-1,
> > > TIMESTAMP=0 }`: the broker will either look up using the absolute
> > > Timestamp, or logical offset value, depending on the value of
> LookupType.
> > > 3. With the added Attribute for extended timestamp, do we really need
> to
> > > have a sentinel value for an unset timestamp (-1 or Long.MIN_VALUE)?
> > > To make the logic simpler I suggest the attribute is renamed to
> just
> > > Timestamp, and if the Timestamp attribute is set, the Timestamp field
> is
> > > always a proper timestamp. If the bit is not set, no timestamp was
> > > provided.
> > >
> > > /Magnus
> > >
> > >
> > >
> > > Den tors 6 dec. 2018 kl 08:06 skrev Gwen Shapira <g...@confluent.io>:
> > >
> > > > I may be missing something, but why are we using an attribute for
> > > > this? IIRC, we normally bump protocol version to indicate semantic
> > > > changes. If I understand correctly, not using an attribute will allow
> > > > us to not change the message format (just the protocol), which makes
> > > > the upgrade significantly easier (since we don't up/down convert).
> > > >
> > > > Another thing I don't understand: The compatibility map indicates
> that
> > > > NO_TIMESTAMP is now Long.MIN_VALUE, but a bit above that you say that
> > > > -1 semantics does not change.
> > > >
> > > > Last: At around version 1.0 we decide to completely avoid changes
> that
> > > > require a particular order of upgrades (this is why we added the
> > > > versions API, up/down conversion, etc). So I'd like to see if we can
> > > > avoid this here (should be relatively easy?). I'm CCing Magnus,
> > > > because I think he remembers *why* we made this decision in first
> > > > place.
> > > >
> > > > Gwen
> > > > On Thu, Dec 6, 2018 at 4:44 PM Guozhang Wang <wangg...@gmail.com>
> > wrote:
> > > > >
> > > > > Bump up on this thread again: we have two binding votes already and
> > > need
> > > > > another committer to take a look at it and vote.
> > > > >
> > > > >
> > > > > Guozhang
> > > > >
> > > > > On Fri, Oct 19, 2018 at 11:34 AM Konstantin Chukhlomin <
> > > > chuhlo...@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > bump
> > > > > >
> > > > > > On Tue, Jun 12, 2018 at 1:48 PM Bill Bejeck <bbej...@gmail.com>
> > > wrote:
> > > > > >
> > > > > > > +1
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Bill
> > > > > > >
> > > > > > > On Sun, Jun 10, 2018 at 5:32 PM Ted Yu <yuzhih...@gmail.com>
> > > wrote:
> > > > > > >
> > > > > > > > +1
> > > > > > > >
> > > > > > > > On Sun, Jun 10, 2018 at 2:17 PM, Matthias J. Sax <
> > > > > > matth...@confluent.io>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > +1 (binding)
> > > > > > > > >
> > > > > > > > > Thanks for the KIP.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > -Matthias
> > > > > > > > >
> > > > > > > > > On 5/29/18 9:14 AM, Konstantin Chukhlomin wrote:
> > > > > > > > > > Thanks, updated the KIP.
> > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > > > > 228+Negative+record+timestamp+support <
> > > https://cwiki.apache.org/
> > > > > > > > >
> > > > confluence/display/KAFKA/KIP-228+Negative+record+timestamp+support>
> > > > > > > > > >
> > > > > > > > > >> On May 24, 2018, at 1:39 PM, Guozhang Wang <
> > > > wangg...@gmail.com>
> > > > > > > > wrote:
> > > > > > > > > >>
> > > > > > > > > >> Thanks Konstantin, I have a minor comment on the wiki
> > page's
> > > > > > > "Proposed
> > > > > > > > > >> Solution" section:
> > > > > > > > > >>
> > > > > > > > > >> "The solution is to ignore that problem and it is a
> choice
> > > of
> > > > the
> > > > > > > user
> > > > > > > > > >> to..":
> > > > > > > > > >>
> > > > > > > > > >> ----------
> > > > > > > > > >>
> > > > > > > > > >> I'd suggest we rephrase it and admit "-1 is a special
> > value
> > > in
> > > > > > > Kafka,
> > > > > > > > > which
> > > > > > > > > >> means we do not have a valid way to express Wednesday,
> > > > December
> > > > > > 31,
> > > > > > > > 1969
> > > > > > > > > >> 11:59:59 PM UTC." rather than letting users to choose
> how
> > to
> > > > > > > interpret
> > > > > > > > > it,
> > > > > > > > > >> since as mentioned, both at Kafka broker side, as well
> as
> > at
> > > > Kafka
> > > > > > > > > client
> > > > > > > > > >> side (Streams, Producer) we have already using this
> > protocol
> > > > to
> > > > > > set
> > > > > > > -1
> > > > > > > > > as a
> > > > > > > > > >> special value of `unknown`, so users choosing how to
> > > > interpret it
> > > > > > > > freely
> > > > > > > > > >> would lead to confusing behaviors. Instead, we should
> well
> > > > > > document
> > > > > > > > this
> > > > > > > > > >> caveat, and educate users in the document that, given
> this
> > > > > > > situation,
> > > > > > > > if
> > > > > > > > > >> you do need to express Wednesday, December 31, 1969
> > 11:59:59
> > > > PM
> > > > > > UTC,
> > > > > > > > > >> consider shifting it by one millisecond.
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >> Otherwise, I'm +1 on this KIP.
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >> Guozhang
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >> On Wed, May 23, 2018 at 2:40 PM, Konstantin Chukhlomin <
> > > > > > > > > chuhlo...@gmail.com>
> > > > > > > > > >> wrote:
> > > > > > > > > >>
> > > > > > > > > >>> All,
> > > > > > > > > >>>
> > > > > > > > > >>> Thanks for the feedback on KIP-228. I've updated the
> KIP,
> > > and
> > > > > > would
> > > > > > > > > like
> > > > > > > > > >>> to start to start a vote.
> > > > > > > > > >>>
> > > > > > > > > >>> KIP:
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > > > > > > > >>> 228+Negative+record+timestamp+support <
> > > > https://cwiki.apache.org/
> > > > > > > > > >>>
> > > > > >
> confluence/display/KAFKA/KIP-228+Negative+record+timestamp+support>
> > > > > > > > > >>>
> > > > > > > > > >>> Pull Request:
> > > > https://github.com/apache/kafka/pull/5072/files <
> > > > > > > > > >>> https://github.com/apache/kafka/pull/5072/files>
> > > > > > > > > >>>
> > > > > > > > > >>> Discussion Thread: http://mail-archives.apache.
> > > > > > > > > >>>
> org/mod_mbox/kafka-dev/201712.mbox/%3c178138FA-CA30-44A8-
> > > > > > > > > >>> b92a-b966b8ca1...@gmail.com%3e <
> > > http://mail-archives.apache.
> > > > > > > > > >>>
> org/mod_mbox/kafka-dev/201712.mbox/%3C178138FA-CA30-44A8-
> > > > > > > > > >>> b92a-b966b8ca1...@gmail.com%3E>
> > > > > > > > > >>>
> > > > > > > > > >>> Thanks!
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >> --
> > > > > > > > > >> -- Guozhang
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Konstantin Chukhlomin
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > -- Guozhang
> > > >
> > > >
> > > >
> > > > --
> > > > Gwen Shapira
> > > > Product Manager | Confluent
> > > > 650.450.2760 | @gwenshap
> > > > Follow us: Twitter | blog
> > > >
> > >
> >
>
>
> --
> -- Guozhang
>
