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 <cmcc...@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 <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
