+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.kari...@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 <eno.there...@gmail.com> > wrote: > > > +1 (non-binding) > > > > Thanks > > Eno > > > > > On 24 Mar 2017, at 03:37, Matthias J. Sax <matth...@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 <wangg...@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.kari...@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 <wangg...@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 < > > matth...@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 < > mich...@confluent.io > > >>>> > > >>>>>> wrote: > > >>>>>>> > > >>>>>>>> +1 (non-binding) > > >>>>>>>> > > >>>>>>>> Thanks for the KIP! > > >>>>>>>> > > >>>>>>>> On Wed, Mar 1, 2017 at 1:49 PM, Bill Bejeck <bbej...@gmail.com> > > >>>>> wrote: > > >>>>>>>> > > >>>>>>>>> +1 > > >>>>>>>>> > > >>>>>>>>> Thanks > > >>>>>>>>> Bill > > >>>>>>>>> > > >>>>>>>>> On Wed, Mar 1, 2017 at 5:06 AM, Eno Thereska < > > >>>> eno.there...@gmail.com > > >>>>>> > > >>>>>>>>> wrote: > > >>>>>>>>> > > >>>>>>>>>> +1 (non binding). > > >>>>>>>>>> > > >>>>>>>>>> Thanks > > >>>>>>>>>> Eno > > >>>>>>>>>>> On 28 Feb 2017, at 17:22, Matthias J. Sax < > > >>> matth...@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.kari...@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 >
