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 >>>>>>> >>>>>> >>>>> >>>> >>>> >>> >> > > >
signature.asc
Description: OpenPGP digital signature
