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
