kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthias J. Sax" <matth...@confluent.io>
Subject Re: [VOTE] KIP-243: Make ProducerConfig and ConsumerConfig constructors public
Date Fri, 12 Jan 2018 02:09:39 GMT
+1

I am closing this vote as accepted with 5 non-binding +1 (Ted, Vahid,
Tom, Bill, Matthias) and 3 binding +1 (Guozhang, Jason, Ewen) votes.

Thanks a lot.


-Matthias


On 1/8/18 1:29 PM, Matthias J. Sax wrote:
> We cannot simply change to <String,?> without breaking a lot of code and
> making it a nightmare to fix it up... :(
> 
> I will leave the VOTE open for some more time if people still want to
> comment. Otherwise, we would have 3 bindings vote now.
> 
> 
> -Matthias
> 
> On 1/8/18 10:58 AM, Ewen Cheslack-Postava wrote:
>> +1 (binding), though I still think the Map should be <String, ?> instead of
>> <?, ?>.
>>
>> I also think its better to just expose the defaults as constants on the
>> class. Apparently there was discussion of this before and the concern is
>> that people write code that rely on the default configs and we might break
>> their code if we change it. I don't really buy this as using the constant
>> allows you to to symbolically reference the value rather than making your
>> own copy of it. Usually if we change a default like that there is an
>> important reason why and having the old copied value might be worse than
>> having the value change out from under you. Having the defaults explicitly
>> exposed can also be helpful when writing tests sometimes.
>>
>> -Ewen
>>
>> On Wed, Jan 3, 2018 at 9:30 AM, Colin McCabe <cmccabe@apache.org> wrote:
>>
>>> On Thu, Dec 21, 2017, at 10:28, 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.
>>>
>>> Yeah, that would be good.  The builder idea would definitely make it a lot
>>> easier to configure clients programmatically.
>>>
>>> I do wonder if there are some cross-version compatibility issues here.  If
>>> there's any configuration that needs to be set by the client, but then
>>> propagated to the broker to be applied, the validation of that
>>> configuration really needs to be done by the broker itself.  The client
>>> code doesn't know the broker version, so it can't validate these configs.
>>> One example is topic configurations (although those are not set by
>>> ProducerConfig).  I'm not sure how big of an issue this is with our current
>>> configurations.
>>>
>>> Another problem here is that all these builder functions become API, and
>>> cannot easily be changed.  So if we want to change a configuration key that
>>> formerly accepted an int to accept a long, it will be difficult to do
>>> that.  We would have to add a new function with a separate name.
>>>
>>> best,
>>> Colin
>>>
>>>
>>>>
>>>> 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 <bbejeck@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 <yuzhihong@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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>
>>
> 


Mime
View raw message