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