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 > > >>>>> > > >>>> > > >>> > > >> > > >> > > > > > > > >
