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
