Thanks for the feedback. I updated the KIP and PR accordingly.
-Matthias On 1/2/18 9:18 PM, Ewen Cheslack-Postava wrote: > Builders have historically seen some resistance in the project (by some > notable original authors...) but they've been increasingly making their way > in -- SchemaBuilder in Connect, I believe some small streams APIs, > AdminClient stuff. The general trend seems to be towards more fluent APIs. > > Regarding the KIP, I'm not sure why the APIs are currently Map<?, ?>, but > this seems wrong. We have a mess of old Properties compatibility (which we > seem to have dragged into the new producer/consumer APIs), but we could at > least make these Map<String, ?>. (I prefer just doing Map<String, String> > since we go through a parse phase regardless of the types, but didn't > manage to make that happen within Connect.) > > Otherwise, the general idea seems fine to me. > > -Ewen > > On Thu, Dec 21, 2017 at 2:28 PM, Jason Gustafson <ja...@confluent.io> wrote: > >> I didn't sense much resistance in that thread, just an effort to keep the >> streams and core client config APIs consistent ;). >> >> I'd prefer seeing a KIP for a more general improvement, but this change >> seems harmless and improves consistency between the clients, so +1 from me. >> >> -Jason >> >> On Thu, Dec 21, 2017 at 11:19 AM, Matthias J. Sax <matth...@confluent.io> >> wrote: >> >>> I personally love the builder pattern idea. There was some push back in >>> the past though from some people. >>> >>> cf https://issues.apache.org/jira/browse/KAFKA-4436 >>> >>> Happy to propose the builder pattern but than we should have a proper >>> DISCUSS thread. Maybe we do this as a follow up and just do this KIP >> as-is? >>> >>> >>> -Matthias >>> >>> On 12/21/17 10:28 AM, Jason Gustafson wrote: >>>> Hey Matthias, >>>> >>>> Let me suggest an alternative. As you have mentioned, these config >>> classes >>>> do not give users much benefit currently. Maybe we change that? I think >>>> many users would appreciate having a builder for configuration since it >>>> provides type safety and is generally a much friendlier pattern to work >>>> with programmatically. Users could then do something like this: >>>> >>>> ConsumerConfig config = ConsumerConfig.newBuilder() >>>> .setBootstrapServers("localhost:9092") >>>> .setGroupId("group") >>>> .setRequestTimeout(15, TimeUnit.SECONDS) >>>> .build(); >>>> >>>> Consumer consumer = new KafkaConsumer(config); >>>> >>>> An additional benefit of this is that it gives us a better way to >> expose >>>> config deprecations. In any case, it would make it less odd to expose >> the >>>> public constructor without giving users anything useful to do with the >>>> class. >>>> >>>> What do you think? >>>> >>>> -Jason >>>> >>>> On Wed, Dec 20, 2017 at 5:59 PM, Matthias J. Sax < >> matth...@confluent.io> >>>> wrote: >>>> >>>>> It's tailored for internal usage. I think client constructors don't >>>>> benefit from accepting those config objects. We just want to be able >> to >>>>> access the default values for certain parameters. >>>>> >>>>> From a user point of view, it's actually boiler plate code if you pass >>>>> in a config object instead of a plain Properties object because the >>>>> config object itself is immutable. >>>>> >>>>> I actually create a JIRA to remove the constructors from KafkaStreams >>>>> that do accept StreamsConfig for exact this reason: >>>>> https://issues.apache.org/jira/browse/KAFKA-6386 >>>>> >>>>> >>>>> -Matthias >>>>> >>>>> >>>>> On 12/20/17 3:33 PM, Jason Gustafson wrote: >>>>>> Hi Matthias, >>>>>> >>>>>> Isn't it a little weird to make these constructors public but not >> also >>>>>> expose the corresponding client constructors that use them? >>>>>> >>>>>> -Jason >>>>>> >>>>>> On Tue, Dec 19, 2017 at 9:30 AM, Bill Bejeck <bbej...@gmail.com> >>> wrote: >>>>>> >>>>>>> +1 >>>>>>> >>>>>>> On Tue, Dec 19, 2017 at 12:09 PM, Guozhang Wang <wangg...@gmail.com >>> >>>>>>> wrote: >>>>>>> >>>>>>>> +1 >>>>>>>> >>>>>>>> On Tue, Dec 19, 2017 at 1:49 AM, Tom Bentley < >> t.j.bent...@gmail.com> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> +1 >>>>>>>>> >>>>>>>>> On 18 December 2017 at 23:28, Vahid S Hashemian < >>>>>>>> vahidhashem...@us.ibm.com >>>>>>>>>> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> +1 >>>>>>>>>> >>>>>>>>>> Thanks for the KIP. >>>>>>>>>> >>>>>>>>>> --Vahid >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> From: Ted Yu <yuzhih...@gmail.com> >>>>>>>>>> To: dev@kafka.apache.org >>>>>>>>>> Date: 12/18/2017 02:45 PM >>>>>>>>>> Subject: Re: [VOTE] KIP-243: Make ProducerConfig and >>>>>>>>> ConsumerConfig >>>>>>>>>> constructors public >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> +1 >>>>>>>>>> >>>>>>>>>> nit: via "copy and past" an 'e' is missing at the end. >>>>>>>>>> >>>>>>>>>> On Mon, Dec 18, 2017 at 2:38 PM, Matthias J. Sax < >>>>>>>> matth...@confluent.io> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> I want to propose the following KIP: >>>>>>>>>>> >>>>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__cwiki. >>>>>>>>>> apache.org_confluence_display_KAFKA_KIP-2D&d=DwIBaQ&c=jf_ >>>>>>>>>> iaSHvJObTbx-siA1ZOg&r=Q_itwloTQj3_xUKl7Nzswo6KE4Nj- >>>>>>>>>> kjJc7uSVcviKUc&m=JToRX4-HeVsRoOekIz18ht-YLMe-T21MttZTgbxB4ag&s= >>>>>>>>>> 6aZjPCc9e00raokVPKvx1BxwDOHyCuKNgtBXPMeoHy4&e= >>>>>>>>>> >>>>>>>>>>> 243%3A+Make+ProducerConfig+and+ConsumerConfig+ >> constructors+public >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> This is a rather straight forward change, thus I skip the >> DISCUSS >>>>>>>>>>> thread and call for a vote immediately. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -Matthias >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> -- Guozhang >>>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>>> >>> >>> >> >
signature.asc
Description: OpenPGP digital signature
