Re: [VOTE] KIP-123: Allow per stream/table timestamp extractor

[Ewen Cheslack-Postava <ew...@confluent.io>]

+1 (binding)

Left a few more comments in the DISCUSS thread, but nothing that would
affect voting. Might also want to note somewhere in the KIP wiki that this
interacts with KIP-120. Not critical, but it might help someone reading the
KIPs for 0.11 figure out that they should actually be using the APIs via a
new class instead of the one mentioned in the KIP since KStreamBuilder will
be deprecated.

-Ewen

On Fri, Mar 24, 2017 at 9:13 AM, Jeyhun Karimov <je...@gmail.com>
wrote:

> Thanks for the comments Matthias. I updated the KIP and PR.
>
> Cheers,
> Jeyhun
>
> On Fri, Mar 24, 2017 at 8:34 AM Eno Thereska <en...@gmail.com>
> wrote:
>
> > +1 (non-binding)
> >
> > Thanks
> > Eno
> >
> > > On 24 Mar 2017, at 03:37, Matthias J. Sax <ma...@confluent.io>
> wrote:
> > >
> > > Thanks Jeyhun.
> > >
> > > Can you also update the KIP accordingly. It must contain all changes to
> > > public API. Thus, list all parameters that get deprecated and newly
> > > added. And add a sentence about backward compatibility.
> > >
> > >
> > > -Matthias
> > >
> > > On 3/23/17 3:16 AM, Jeyhun Karimov wrote:
> > >> Sorry for a super late update. I made an update on related PR.
> > >>
> > >> Cheers,
> > >> Jeyhun
> > >>
> > >> On Wed, Mar 22, 2017 at 9:09 PM Guozhang Wang <wa...@gmail.com>
> > wrote:
> > >>
> > >>> Jeyhun,
> > >>>
> > >>> Could you update the status of this KIP since it has been some time
> > since
> > >>> the last vote?
> > >>>
> > >>> I'm +1 besides the minor comments mentioned above.
> > >>>
> > >>>
> > >>> Guozhang
> > >>>
> > >>>
> > >>> On Mon, Mar 6, 2017 at 3:14 PM, Jeyhun Karimov <je.karimov@gmail.com
> >
> > >>> wrote:
> > >>>
> > >>>> Sorry I was late. I will update javadocs in related methods to
> > emphasize
> > >>>> that TimestampExtractor is stateless.
> > >>>>
> > >>>> Cheers,
> > >>>> Jeyhun
> > >>>>
> > >>>> On Mon, Mar 6, 2017 at 8:17 PM Guozhang Wang <wa...@gmail.com>
> > wrote:
> > >>>>
> > >>>>> 1) Sounds good.
> > >>>>>
> > >>>>> 2) Yeah what I meant is to emphasize that TimestampExtractor to be
> > >>>>> stateless in the docs somewhere.
> > >>>>>
> > >>>>>
> > >>>>> Guozhang
> > >>>>>
> > >>>>>
> > >>>>> On Sun, Mar 5, 2017 at 4:27 PM, Matthias J. Sax <
> > matthias@confluent.io
> > >>>>
> > >>>>> wrote:
> > >>>>>
> > >>>>>> Guozhang,
> > >>>>>>
> > >>>>>> about renaming the config parameters. I like this idea, but want
> to
> > >>>>>> point out, that this change should be done in a backward
> compatible
> > >>>> way.
> > >>>>>> Thus, we need to keep (and only deprecate) the current parameter
> > >>> names.
> > >>>>>>
> > >>>>>> I am not sure about (2)? What do you worry about? Using a
> "stateful
> > >>>>>> extractor"? -- this would be an antipattern IMHO. We can clarify
> > >>> that a
> > >>>>>> TimestampExtrator should be stateless though (even if this should
> be
> > >>>>>> clear).
> > >>>>>>
> > >>>>>>
> > >>>>>> -Matthias
> > >>>>>>
> > >>>>>>
> > >>>>>> On 3/4/17 6:36 PM, Guozhang Wang wrote:
> > >>>>>>> Jeyhun,
> > >>>>>>>
> > >>>>>>> Thanks for proposing this KIP! And sorry for getting late in the
> > >>>>>> discussion.
> > >>>>>>>
> > >>>>>>> I have a general suggestion not directly related to this KIP and
> a
> > >>>>> couple
> > >>>>>>> of comments for this KIP here:
> > >>>>>>>
> > >>>>>>> I agree with Mathieu's observation, partly because we are now
> > >>> having
> > >>>>> lots
> > >>>>>>> of overloaded functions both in the DSL and in PAPI, and it would
> > >>> be
> > >>>>>> quite
> > >>>>>>> confusing to users. As Matthias mentioned we do have some plans
> to
> > >>>>>> refactor
> > >>>>>>> this API, but just wanted to point it out that this KIP may
> likely
> > >>>> urge
> > >>>>>> us
> > >>>>>>> to do the API refactoring sooner than planned. My personal
> > >>> preference
> > >>>>>> would
> > >>>>>>> be doing that the next release (i.e. 0.11.0.0 in June).
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> Now some detailed comments:
> > >>>>>>>
> > >>>>>>> 1. I'd suggest change TIMESTAMP_EXTRACTOR_CLASS_CONFIG to
> > >>>>>>> "default.timestamp.extractor" or "global.timestamp.extractor"
> (also
> > >>>> the
> > >>>>>>> Java variable name can be changed accordingly) along with this
> > >>>> change.
> > >>>>> In
> > >>>>>>> addition, maybe we can piggy-backing this to also rename
> > >>>>>>> KEY_SERDE_CLASS_CONFIG/VALUE_SERDE_CLASS_CONFIG to
> "default.key.."
> > >>>> etc
> > >>>>>> in
> > >>>>>>> this KIP.
> > >>>>>>>
> > >>>>>>> 2. Another thing we should consider, is that since now we could
> > >>>>>> potentially
> > >>>>>>> use multiple timestamp extractor instances than a single one,
> this
> > >>>> may
> > >>>>> be
> > >>>>>>> breaking if user's customization did some global bookkeeping
> based
> > >>> on
> > >>>>> the
> > >>>>>>> previous assumption (maybe a wild thought but e.g. keeping track
> > >>> some
> > >>>>>>> global counts in the extractor as a local variable). We need to
> > >>>> clarify
> > >>>>>>> this change in the javadoc and also potentially in the upgrade
> web
> > >>>> doc
> > >>>>>>> sections.
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> Guozhang
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> On Wed, Mar 1, 2017 at 6:09 AM, Michael Noll <
> michael@confluent.io
> > >>>>
> > >>>>>> wrote:
> > >>>>>>>
> > >>>>>>>> +1 (non-binding)
> > >>>>>>>>
> > >>>>>>>> Thanks for the KIP!
> > >>>>>>>>
> > >>>>>>>> On Wed, Mar 1, 2017 at 1:49 PM, Bill Bejeck <bb...@gmail.com>
> > >>>>> wrote:
> > >>>>>>>>
> > >>>>>>>>> +1
> > >>>>>>>>>
> > >>>>>>>>> Thanks
> > >>>>>>>>> Bill
> > >>>>>>>>>
> > >>>>>>>>> On Wed, Mar 1, 2017 at 5:06 AM, Eno Thereska <
> > >>>> eno.thereska@gmail.com
> > >>>>>>
> > >>>>>>>>> wrote:
> > >>>>>>>>>
> > >>>>>>>>>> +1 (non binding).
> > >>>>>>>>>>
> > >>>>>>>>>> Thanks
> > >>>>>>>>>> Eno
> > >>>>>>>>>>> On 28 Feb 2017, at 17:22, Matthias J. Sax <
> > >>> matthias@confluent.io
> > >>>>>
> > >>>>>>>>> wrote:
> > >>>>>>>>>>>
> > >>>>>>>>>>> +1
> > >>>>>>>>>>>
> > >>>>>>>>>>> Thanks a lot for the KIP!
> > >>>>>>>>>>>
> > >>>>>>>>>>> -Matthias
> > >>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>> On 2/28/17 1:35 AM, Damian Guy wrote:
> > >>>>>>>>>>>> Thanks for the KIP Jeyhun!
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> +1
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> On Tue, 28 Feb 2017 at 08:59 Jeyhun Karimov <
> > >>>> je.karimov@gmail.com
> > >>>>>>
> > >>>>>>>>>> wrote:
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>> Dear community,
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> I'd like to start the vote for KIP-123:
> > >>>>>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage.
> > >>>>>>>>>> action?pageId=68714788
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Cheers,
> > >>>>>>>>>>>>> Jeyhun
> > >>>>>>>>>>>>> --
> > >>>>>>>>>>>>> -Cheers
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>> Jeyhun
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>> --
> > >>>>> -- Guozhang
> > >>>>>
> > >>>> --
> > >>>> -Cheers
> > >>>>
> > >>>> Jeyhun
> > >>>>
> > >>>
> > >>>
> > >>>
> > >>> --
> > >>> -- Guozhang
> > >>>
> > >
> >
> > --
> -Cheers
>
> Jeyhun
>