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
