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
