You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Ewen Cheslack-Postava <ew...@confluent.io> on 2018/01/03 05:18:29 UTC
Re: [VOTE] KIP-243: Make ProducerConfig and ConsumerConfig
constructors public
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 <ma...@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 <
> matthias@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 <bb...@gmail.com>
> > wrote:
> > >>>
> > >>>> +1
> > >>>>
> > >>>> On Tue, Dec 19, 2017 at 12:09 PM, Guozhang Wang <wangguoz@gmail.com
> >
> > >>>> wrote:
> > >>>>
> > >>>>> +1
> > >>>>>
> > >>>>> On Tue, Dec 19, 2017 at 1:49 AM, Tom Bentley <
> t.j.bentley@gmail.com>
> > >>>>> wrote:
> > >>>>>
> > >>>>>> +1
> > >>>>>>
> > >>>>>> On 18 December 2017 at 23:28, Vahid S Hashemian <
> > >>>>> vahidhashemian@us.ibm.com
> > >>>>>>>
> > >>>>>> wrote:
> > >>>>>>
> > >>>>>>> +1
> > >>>>>>>
> > >>>>>>> Thanks for the KIP.
> > >>>>>>>
> > >>>>>>> --Vahid
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> From: Ted Yu <yu...@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 <
> > >>>>> matthias@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
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >>
> > >
> >
> >
>
Re: [VOTE] KIP-243: Make ProducerConfig and ConsumerConfig
constructors public
Posted by "Matthias J. Sax" <ma...@confluent.io>.
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 <ma...@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 <
>> matthias@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 <bb...@gmail.com>
>>> wrote:
>>>>>>
>>>>>>> +1
>>>>>>>
>>>>>>> On Tue, Dec 19, 2017 at 12:09 PM, Guozhang Wang <wangguoz@gmail.com
>>>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> +1
>>>>>>>>
>>>>>>>> On Tue, Dec 19, 2017 at 1:49 AM, Tom Bentley <
>> t.j.bentley@gmail.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> +1
>>>>>>>>>
>>>>>>>>> On 18 December 2017 at 23:28, Vahid S Hashemian <
>>>>>>>> vahidhashemian@us.ibm.com
>>>>>>>>>>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> +1
>>>>>>>>>>
>>>>>>>>>> Thanks for the KIP.
>>>>>>>>>>
>>>>>>>>>> --Vahid
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> From: Ted Yu <yu...@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 <
>>>>>>>> matthias@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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>