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
