I agree. I would really like to see a builder interface here. If you changed
the KIP to add this, you'd have to do the vote again, but I think it would be
well worth it.
best,
Colin
On Thu, May 12, 2022, at 00:13, Bruno Cadonna wrote:
> Hi Francois,
>
> Modifying this KIP or starting a new one is your call.
>
> I guess the idea of the builder might cause some more discussions. So if
> you want to get a working version released as soon as possible, I would
> opt to get the current KIP implemented and create a new one for the
> builder. If time is not an issue I would modify the current KIP.
>
> But as I said: It's your call.
>
> Best,
> Bruno
>
> On 11.05.22 11:01, François Rosière wrote:
>> To be clear, there is no problem for me to update the current KIP with the
>> builder approach.
>> It's not a lot of work in terms of code.
>> So, up to you. Let me know and I will do the necessary to go in one or the
>> other direction...
>> Thanks again for the feedbacks.
>>
>> Le mer. 11 mai 2022 à 10:52, François Rosière <francois.rosi...@gmail.com>
>> a écrit :
>>
>>> Hello,
>>>
>>> Builder is clearly the way to go for future releases of Kafka.
>>>
>>> If we align streams, we would have 3 builders
>>>
>>> new ConsumerBuilder<String, MyPojo>()
>>> .withKeyDeserializer(<KEY_DESERIALIZER>)
>>> .withValueDeserializer(<VALUE_DESERIALIZER>)
>>> .withInterceptors(<LIST_OF_INTERCEPTORS>)
>>> .withMetricsReporter(<METRICS_REPORTER>)
>>> .build(<MAP_OR_PROPERTIES_OR_CONFIG>);
>>>
>>> new ProducerBuilder<String, MyPojo>()
>>> .withKeySerializer(<KEY_SERIALIZER>)
>>> .withValueSerializer(<VALUE_SERIALIZER>)
>>> .withInterceptors(<LIST_OF_INTERCEPTORS>)
>>> .withPartitioner(<PARTITIONER>)
>>> .withMetricsReporter(<METRICS_REPORTER>)
>>> .build(<MAP_OR_PROPERTIES_OR_CONFIG>);
>>>
>>> new KafkaStreamsBuilder(<TOPOLOGY>)
>>> .withProducerInterceptors(<LIST_OF_PRODUER_INTERCEPTORS>)
>>> .withConsumerInterceptors(<LIST_OF_CONSUMER_INTERCEPTORS>)
>>> .withTime(<TIME>)
>>> .withKafkaClientSupplier(<KAFKA_CLIENT_SUPPLIER>)
>>> .withMetricsReporter(<METRICS_REPORTER>)
>>> .build(<MAP_OR_PROPERTIES_OR_CONFIG>);
>>>
>>> The builder property would always override the configuration "instances".
>>> There is maybe other methods to add to the builders...
>>> The map, properties or config could be given to the constructor of the
>>> builder instead of the build method...
>>> At the end, we may only keep one single constructor in the Producer,
>>> Consumer and KafkaStreams obects.
>>>
>>> @Chris, @Bruno, thank you for your replies and proposals. Do you want I
>>> create another KIP explaining the builder approach or do you prefer to do
>>> it?
>>>
>>> Kr,
>>>
>>> F.
>>>
>>>
>>> Le mer. 11 mai 2022 à 09:46, Bruno Cadonna <cado...@apache.org> a écrit :
>>>
>>>> Hi Francois and Chris,
>>>>
>>>> I find the idea of the builder interesting.
>>>>
>>>> I think we should go ahead with the current KIP as it is to allow
>>>> Francois to fix his issue soon. If one of you or both want to push
>>>> forward the builder idea, feel free to create a new KIP and discuss it
>>>> with the community.
>>>>
>>>> Regarding Francois' questions:
>>>>
>>>> 3. If the existing constructors should be removed, they need to be
>>>> marked as deprecated and removed in one of the next major releases.
>>>>
>>>> 5. Yes, I think Streams should be aligned.
>>>>
>>>> Both questions should be discussed in the context of a new KIP about the
>>>> builder idea.
>>>>
>>>> Best,
>>>> Bruno
>>>>
>>>> On 11.05.22 04:24, Chris Egerton wrote:
>>>>> Hi Francois,
>>>>>
>>>>> Thanks for your thoughts. I think it's worth noting that in regards to
>>>> item
>>>>> 2, it's possible to explicitly declare the type parameters for a builder
>>>>> without capturing it in a variable; for example:
>>>>>
>>>>> KafkaProducer<String, Integer> p = new Builder<String, Integer>(...)
>>>>> .withKeySerializer(new StringSerializer())
>>>>> .withValueSerializer(new IntegerSerializer())
>>>>> .build();
>>>>>
>>>>> That aside, given the three binding votes already cast on the vote
>>>> thread,
>>>>> it's probably too late to be worth changing direction at this point.
>>>> Thanks
>>>>> for entertaining the proposal, and congratulations on your KIP!
>>>>>
>>>>> Cheers,
>>>>>
>>>>> Chris
>>>>>
>>>>> On Tue, May 10, 2022 at 5:33 PM François Rosière <
>>>> francois.rosi...@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Hello Chris,
>>>>>>
>>>>>> Thanks for the feedback. Builders is definitely the pattern to apply
>>>> when
>>>>>> an object needs to be built using different arguments/combinations.
>>>>>>
>>>>>> Here are my thoughts about the proposal:
>>>>>>
>>>>>> 1. The builder should only expose meaningful methods for the users
>>>> such as
>>>>>> the interceptors, the serializer/deserializer, partitioner, etc. A
>>>> method
>>>>>> like the configured instances is internal and should not be exposed if
>>>> we
>>>>>> don't want to expose the config itself. Using this internal method is
>>>> the
>>>>>> only way to solve the issue if the config is exposed.
>>>>>>
>>>>>> 2. As the key and value types are not given, a variable will need to be
>>>>>> created for the builder before being used. Otherwise, there is no way
>>>> to
>>>>>> infer the type correctly. Breaks a bit the inline usage with DSL style.
>>>>>>
>>>>>> 3. What about existing constructors, they would need to stay to keep
>>>>>> compatibility with existing o could they be removed in benefit of the
>>>>>> builder?
>>>>>>
>>>>>> 4. Having an access to the config also gives a way to also fine tune
>>>> other
>>>>>> aspects such as the logging related to the config. Log unused, skip
>>>> some
>>>>>> properties, etc.
>>>>>>
>>>>>> 5. What about streams? Shouldn't it be aligned?
>>>>>>
>>>>>> So, to summarise, the KIP was a best effort solution to support already
>>>>>> configured instances related to both the producer and the consumer.
>>>>>> The builder will work, it's just a matter of deciding the best
>>>> approach...
>>>>>> for me, both solutions are fine, I just need a way to inject already
>>>>>> configured dependencies into the producers and consumers.
>>>>>>
>>>>>> If we conclude, I will drop a PR on Github.
>>>>>>
>>>>>> Kr,
>>>>>>
>>>>>> F.
>>>>>>
>>>>>> Le mar. 10 mai 2022 à 15:01, Chris Egerton <fearthecel...@gmail.com> a
>>>>>> écrit :
>>>>>>
>>>>>>> Hi Francois,
>>>>>>>
>>>>>>> Thanks for the KIP! I sympathize with the issue you're facing and with
>>>>>>> John's reluctance to let perfect be the enemy of good, and if KIP
>>>> freeze
>>>>>>> were tomorrow, I think this would be good enough. Given that we still
>>>>>> have
>>>>>>> some time to work with, I'd like to propose an alternative approach
>>>> and
>>>>>> see
>>>>>>> what your thoughts are.
>>>>>>>
>>>>>>> There are a few issues with the current client APIs that are closely
>>>>>>> related to the KIP:
>>>>>>> 1. Too many constructors (there are currently four each for
>>>> KafkaProducer
>>>>>>> and KafkaConsumer, yet they all do basically the same thing)
>>>>>>> 2. Lack of type safety with interceptors (you have no way to enforce
>>>> at
>>>>>>> compile time that your ProducerInterceptor<String, Integer> is used
>>>> with
>>>>>> a
>>>>>>> Serializer<String> and Serializer<Integer>, for example)
>>>>>>> 3. Inflexibility and inconsistency with instantiation of pluggable
>>>>>>> interfaces (you can bring your own (de)serializers, but everything
>>>> else
>>>>>>> gets instantiated and configured for you at producer/consumer
>>>>>> construction
>>>>>>> time)
>>>>>>>
>>>>>>> The KIP as it exists now will only address item 3, and will exacerbate
>>>>>> item
>>>>>>> 1.
>>>>>>>
>>>>>>> In addition, there are a few new issues introduced by the KIP as it
>>>>>> exists
>>>>>>> now:
>>>>>>> 1. Tighter coupling between the ProducerConfig/ConsumerConfig classes
>>>> and
>>>>>>> the KafkaProducer/KafkaConsumer classes. Any change we make in the
>>>> future
>>>>>>> that breaks either of these config classes in unexpected ways (but
>>>> which
>>>>>>> does not break the KafkaProducer/KafkaConsumer constructors that do
>>>> not
>>>>>>> accept these classes as parameters) will now have a much higher
>>>> chance to
>>>>>>> also break a user's entire producer/consumer application.
>>>>>>> 2. Complexity for users like yourself who would like to override
>>>> behavior
>>>>>>> in a ProducerConfig/ConsumerConfig in order to inject pre-instantiated
>>>>>>> dependencies. The example in the KIP overrides
>>>>>>> AbstractConfig::getConfiguredInstances [1] in order to achieve this.
>>>> But
>>>>>>> there are two other overloaded variants of getConfiguredInstances, and
>>>>>> two
>>>>>>> AbstractConfig::getConfiguredInstance methods that also exist. We'd
>>>>>> either
>>>>>>> need to establish a dependency graph between these methods (e.g., some
>>>>>>> methods are guaranteed to invoke another overloaded variant) as part
>>>> of
>>>>>> the
>>>>>>> public API for the AbstractConfig, or users would need to override
>>>> every
>>>>>>> single one of these methods in order to ensure that their code won't
>>>>>> break
>>>>>>> at runtime after bumping their Kafka version.
>>>>>>>
>>>>>>> I think introducing the builder pattern for KafkaProducer and
>>>>>> KafkaConsumer
>>>>>>> would alleviate all of these issues. As a rough draft of what the API
>>>>>> might
>>>>>>> look like for KafkaProducer:
>>>>>>>
>>>>>>> public class Builder<K, V> {
>>>>>>> private final Map<String, Object> props;
>>>>>>> private Serializer<K> keySerializer;
>>>>>>> private Serializer<V> valueSerializer;
>>>>>>> private List<ProducerInterceptor<K, V>> interceptors;
>>>>>>> private Map<String, Object> configuredInstances;
>>>>>>> private Map<String, List<Object>> configuredInstanceLists;
>>>>>>>
>>>>>>> public Builder(Map<String, Object> props) {
>>>>>>> this.props = props;
>>>>>>> this.interceptors = new ArrayList<>();
>>>>>>> this.configuredInstances = new HashMap<>();
>>>>>>> this.configuredInstanceLists = new HashMap<>();
>>>>>>> }
>>>>>>>
>>>>>>> // Use this serializer, if non-null
>>>>>>> // Will take precedence over any serializer class specified in
>>>> the
>>>>>>> properties for this producer
>>>>>>> public Builder withKeySerializer(Serializer<K> serializer) {
>>>>>>> this.keySerializer = serializer;
>>>>>>> return this;
>>>>>>> }
>>>>>>>
>>>>>>> public Builder withValueSerializer(Serializer<V> serializer) {
>>>>>>> this.valueSerializer = serializer;
>>>>>>> return this;
>>>>>>> }
>>>>>>>
>>>>>>> // Use these interceptors (has no effect if null)
>>>>>>> // Each must already be configured
>>>>>>> // Will be combined with any interceptor classes also specified
>>>> in
>>>>>> the
>>>>>>> properties for this producer
>>>>>>> public Builder withInterceptors(List<ProducerInterceptor<K, V>>
>>>>>>> interceptors) {
>>>>>>> if (interceptors != null) {
>>>>>>> this.interceptors.addAll(interceptors);
>>>>>>> }
>>>>>>> return this;
>>>>>>> }
>>>>>>>
>>>>>>> // Use this plugin instance, if non-null
>>>>>>> // Must already be configured
>>>>>>> // Will take precedence over any plugin class specified for the
>>>> same
>>>>>>> property in the properties for this producer (wording here needs work
>>>> but
>>>>>>> you get the idea)
>>>>>>> public Builder withConfiguredInstance(String property, Object
>>>>>> instance)
>>>>>>> {
>>>>>>> this.configuredInstances.put(property, instance);
>>>>>>> return this;
>>>>>>> }
>>>>>>>
>>>>>>> // Use these plugin instances (has no effect if null)
>>>>>>> // Each must already be configured
>>>>>>> // Will be combined with any plugin classes also specified for
>>>> the
>>>>>> same
>>>>>>> property in the properties for this consumer
>>>>>>> public Builder withConfiguredInstances(String property,
>>>> List<Object>
>>>>>>> instances) {
>>>>>>> this.configuredInstanceLists.put(property, instances);
>>>>>>> return this;
>>>>>>> }
>>>>>>>
>>>>>>> public KafkaProducer<K, V> build() { ... }
>>>>>>> }
>>>>>>>
>>>>>>> Thoughts?
>>>>>>>
>>>>>>> [1] -
>>>>>>>
>>>>>>>
>>>>>>
>>>> https://kafka.apache.org/31/javadoc/org/apache/kafka/common/config/AbstractConfig.html#getConfiguredInstances(java.lang.String,java.lang.Class,java.util.Map)
>>>>>>>
>>>>>>> Cheers,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On Mon, May 9, 2022 at 4:55 PM Bruno Cadonna <cado...@apache.org>
>>>> wrote:
>>>>>>>
>>>>>>>> Hi Francois,
>>>>>>>>
>>>>>>>> I think you can go ahead and call for votes.
>>>>>>>>
>>>>>>>> Could you please also clean up a little bit the KIP since it has
>>>> still
>>>>>>>> parts that refer to its first version? For example, "Compatibility,
>>>>>>>> Deprecation, and Migration Plan" still mentions only two
>>>> constructors.
>>>>>>>> IMO you can also remove section "Public Interfaces" since it does not
>>>>>>>> contain much information.
>>>>>>>>
>>>>>>>> Best,
>>>>>>>> Bruno
>>>>>>>>
>>>>>>>> On 09.05.22 17:45, Bruno Cadonna wrote:
>>>>>>>>> Hi Francois,
>>>>>>>>>
>>>>>>>>> You can open a PR and people can review it, but it must not be
>>>> merged
>>>>>>>>> until the KIP is approved.
>>>>>>>>>
>>>>>>>>> Best,
>>>>>>>>> Bruno
>>>>>>>>>
>>>>>>>>> On 09.05.22 16:07, François Rosière wrote:
>>>>>>>>>> Can a PR be dropped on Github or do we still need some approval
>>>>>> first?
>>>>>>>>>>
>>>>>>>>>> Le dim. 8 mai 2022 à 06:08, John Roesler <vvcep...@apache.org> a
>>>>>>> écrit
>>>>>>>> :
>>>>>>>>>>
>>>>>>>>>>> Thanks, François!
>>>>>>>>>>>
>>>>>>>>>>> Those changes look good to me.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> -John
>>>>>>>>>>>
>>>>>>>>>>> On Fri, May 6, 2022, at 13:51, François Rosière wrote:
>>>>>>>>>>>> The KIP has been updated to reflect the last discussion
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578#KIP832:Allowcreatingaproducer/consumerusingaproducer/consumerconfig-ProposedChanges
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Le ven. 6 mai 2022 à 20:44, François Rosière
>>>>>>>>>>>> <francois.rosi...@gmail.com>
>>>>>>>>>>> a
>>>>>>>>>>>> écrit :
>>>>>>>>>>>>
>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>
>>>>>>>>>>>>> No problem to also add a constructor taking the StreamsConfig in
>>>>>>> the
>>>>>>>>>>>>> TopologyTestDriver.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Summary about the changes to apply:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - Create 2 new constructors in KafkaProducer
>>>>>>>>>>>>> - Create a new constructor in KafkaConsumer and increase de
>>>>>>>>>>> visibility
>>>>>>>>>>>>> of an existing one
>>>>>>>>>>>>> - Create a new constructor in TopologyTestDriver
>>>>>>>>>>>>>
>>>>>>>>>>>>> Kr,
>>>>>>>>>>>>>
>>>>>>>>>>>>> F.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Le ven. 6 mai 2022 à 16:57, John Roesler <vvcep...@apache.org>
>>>> a
>>>>>>>> écrit
>>>>>>>>>>> :
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks for the KIP, François!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'm generally in favor of your KIP, since you're
>>>>>>>>>>>>>> proposing to follow the existing pattern of the
>>>>>>>>>>>>>> constructors for both Producer and Consumer,
>>>>>>>>>>>>>> but with the config object instead of Properties
>>>>>>>>>>>>>> or Map configs. Also, because we already have
>>>>>>>>>>>>>> this pattern in Streams, and we are just
>>>>>>>>>>>>>> extending it to Producer and Consumer.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Following on the KIP-378 discussion, I do still think
>>>>>>>>>>>>>> this is somewhat of an abuse of the Config objects,
>>>>>>>>>>>>>> and it would be better to have a formal dependency
>>>>>>>>>>>>>> injection interface, but I also don't want to let perfect
>>>>>>>>>>>>>> be the enemy of good. Since it looks like this approach
>>>>>>>>>>>>>> works, and there is also some precedent for it already,
>>>>>>>>>>>>>> I'd be inclined to approve it.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Since KIP-378 didn't make it over the finish line, and it
>>>>>>>>>>>>>> seems like a small expansion to your proposal, do you
>>>>>>>>>>>>>> mind also adding the StreamsConfig to the
>>>>>>>>>>>>>> TopologyTestDriver constructors? That way, we can go
>>>>>>>>>>>>>> ahead and resolve both KIPs at once.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thank you,
>>>>>>>>>>>>>> -John
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Fri, May 6, 2022, at 06:06, François Rosière wrote:
>>>>>>>>>>>>>>> To stay consistent with existing code, we should simply add 2
>>>>>>>>>>>>>> constructors.
>>>>>>>>>>>>>>> One with ser/deser and one without.
>>>>>>>>>>>>>>> So that, users have the choice to use one or the other.
>>>>>>>>>>>>>>> I updated the KIP accordingly.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Le ven. 6 mai 2022 à 12:55, François Rosière <
>>>>>>>>>>>>>> francois.rosi...@gmail.com> a
>>>>>>>>>>>>>>> écrit :
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On the other hand, the KafkaConsumer constructor with a
>>>>>> config +
>>>>>>>>>>>>>>>> serializer and deserializer already exists but is not public.
>>>>>>>>>>>>>>>> It would also complexify a bit the caller to not have the
>>>>>>>>>>>>>>>> serializer/deserializer exposed at constructor level.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Once the KIP would have been implemented, for streams,
>>>> instead
>>>>>>> of
>>>>>>>>>>>>>> having a
>>>>>>>>>>>>>>>> custom config (already possible), I may simply define a
>>>> custom
>>>>>>>>>>>>>>>> KafkaClientSupplier reusing the custom configs of both the
>>>>>>>> producer
>>>>>>>>>>>>>> and the
>>>>>>>>>>>>>>>> consumer.
>>>>>>>>>>>>>>>> This supplier currently creates producers and consumers using
>>>>>>> the
>>>>>>>>>>>>>>>> constructors with a map of config + serializer/deserializer.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So, it seems it's easier to have the constructor with 3
>>>>>>>> parameters.
>>>>>>>>>>>>>> But in
>>>>>>>>>>>>>>>> any case, it will work if the config can be accessed...
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Le ven. 6 mai 2022 à 12:14, François Rosière <
>>>>>>>>>>>>>> francois.rosi...@gmail.com>
>>>>>>>>>>>>>>>> a écrit :
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> We may create a constructor with a single parameter which is
>>>>>>> the
>>>>>>>>>>>>>> config
>>>>>>>>>>>>>>>>> but then, I would need to give the serializer/deserializer
>>>> by
>>>>>>>> also
>>>>>>>>>>>>>>>>> overriding the config.
>>>>>>>>>>>>>>>>> Like I would do for the interceptors.
>>>>>>>>>>>>>>>>> So, no real opinion on that, both solutions are ok for me.
>>>>>>>>>>>>>>>>> Maybe easier to take the approach of the single parameter.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hope it respond to the question.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Kr,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> F.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Le ven. 6 mai 2022 à 11:59, Bruno Cadonna <
>>>>>> cado...@apache.org>
>>>>>>> a
>>>>>>>>>>>>>> écrit :
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Hi Francois,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thank you for updating the KIP!
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Now the motivation of the KIP is much clearer.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I would still be interested in:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> >> 2. Why do you only want to change/add the
>>>> constructors
>>>>>>> that
>>>>>>>>>>> take
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>> >> properties objects and de/serializers and you do not
>>>>>> also
>>>>>>>>>>> want to
>>>>>>>>>>>>>>>>>> >> add/change the constructors that take only the
>>>>>>> properties?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Best,
>>>>>>>>>>>>>>>>>> Bruno
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 05.05.22 23:15, François Rosière wrote:
>>>>>>>>>>>>>>>>>>> Hello Bruno,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> The KIP as been updated. Feel free to give more feedbacks
>>>>>>> and I
>>>>>>>>>>>>>> will
>>>>>>>>>>>>>>>>>>> complete accordingly.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Kr,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> F.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Le jeu. 5 mai 2022 à 22:22, Bruno Cadonna <
>>>>>>> cado...@apache.org>
>>>>>>>>>>> a
>>>>>>>>>>>>>>>>>> écrit :
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Hi Francois,
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Thanks for the KIP!
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Here my first feedback:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> 1. Could you please extend the motivation section, so
>>>> that
>>>>>>> it
>>>>>>>>>>> is
>>>>>>>>>>>>>> clear
>>>>>>>>>>>>>>>>>>>> for a non-Spring dev why the change is needed? Usually, a
>>>>>>>>>>>>>> motivation
>>>>>>>>>>>>>>>>>>>> section benefits a lot from an actual example.
>>>>>>>>>>>>>>>>>>>> Extending the motivation section would also make the KIP
>>>>>>> more
>>>>>>>>>>>>>>>>>>>> self-contained which is important IMO since this is kind
>>>>>> of
>>>>>>> a
>>>>>>>>>>> log
>>>>>>>>>>>>>> of
>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>> major changes to Kafka. Descriptions of major changes
>>>>>> should
>>>>>>>>>>> not
>>>>>>>>>>>>>>>>>>>> completely depend on external links (which may become
>>>> dead
>>>>>>> in
>>>>>>>>>>>>>> future).
>>>>>>>>>>>>>>>>>>>> Referencing external resources to point to more details
>>>> or
>>>>>>>> give
>>>>>>>>>>>>>>>>>> context
>>>>>>>>>>>>>>>>>>>> is useful, though.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> 2. Why do you only want to change/add the constructors
>>>>>> that
>>>>>>>>>>> take
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>> properties objects and de/serializers and you do not also
>>>>>>> want
>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>>>> add/change the constructors that take only the
>>>> properties?
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> 3. I found the following stalled KIP whose motivation is
>>>>>>>> really
>>>>>>>>>>>>>>>>>> similar
>>>>>>>>>>>>>>>>>>>> to yours:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-378%3A+Enable+Dependency+Injection+for+Kafka+Streams+handlers
>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> That KIP is also the reason why Kafka Streams still has
>>>>>> the
>>>>>>>>>>>>>>>>>> constructors
>>>>>>>>>>>>>>>>>>>> with the StreamsConfig parameter. Maybe you want to
>>>>>> mention
>>>>>>>>>>> this
>>>>>>>>>>>>>> KIP
>>>>>>>>>>>>>>>>>> in
>>>>>>>>>>>>>>>>>>>> yours or even incorporate the remaining topology test
>>>>>> driver
>>>>>>>>>>> API
>>>>>>>>>>>>>>>>>> changes
>>>>>>>>>>>>>>>>>>>> in your KIP.
>>>>>>>>>>>>>>>>>>>> Some related links:
>>>>>>>>>>>>>>>>>>>> -
>>>>>>>>>>>>>>
>>>>>> https://github.com/apache/kafka/pull/5344#issuecomment-413350338
>>>>>>>>>>>>>>>>>>>> - https://github.com/apache/kafka/pull/10484
>>>>>>>>>>>>>>>>>>>> - https://issues.apache.org/jira/browse/KAFKA-6386
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Best,
>>>>>>>>>>>>>>>>>>>> Bruno
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 04.05.22 22:26, François Rosière wrote:
>>>>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> KIP-832 has been created to allow implementing Spring
>>>>>>> managed
>>>>>>>>>>>>>>>>>>>> interceptors
>>>>>>>>>>>>>>>>>>>>> for Producers and Consumers.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> At the moment, interceptors are given as configuration
>>>>>>>> classes
>>>>>>>>>>>>>> to the
>>>>>>>>>>>>>>>>>>>>> producer and consumer configurations. So, the idea here
>>>>>>> would
>>>>>>>>>>> be
>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>>>> create
>>>>>>>>>>>>>>>>>>>>> 2 new constructors to allow using a Producer and
>>>> Consumer
>>>>>>>>>>>>>>>>>> configuration
>>>>>>>>>>>>>>>>>>>>> instead of properties or a key value map of
>>>>>> configurations
>>>>>>>>>>>>>> entries.
>>>>>>>>>>>>>>>>>>>>> Interceptors could then be given as instances by
>>>>>>> overriding a
>>>>>>>>>>>>>> config
>>>>>>>>>>>>>>>>>>>> method.
>>>>>>>>>>>>>>>>>>>>> More details can be found in the Spring issue.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>> https://github.com/spring-projects/spring-kafka/issues/2244
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Any feedback, proposal, vote for this KIP would be more
>>>>>>> than
>>>>>>>>>>>>>> welcome.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Kind regards,
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Francois R.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Le lun. 2 mai 2022 à 21:05, François Rosière <
>>>>>>>>>>>>>>>>>> francois.rosi...@gmail.com>
>>>>>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>>>>>>> écrit :
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Kip link:
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578
>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
