Hi Francois, This is a bit of an unusual situation. I think a separate KIP may be the safest route here since this one has already gotten the votes necessary to be included in the next release (as long as it can be implemented and reviewed in time), but it'd probably be best if only one of this KIP and the other one get approved and released. Perhaps you could mention in the separate KIP that it's meant to supersede KIP-832 if it can be approved in time for the 3.3.0 release, and otherwise, we'll move forward with KIP-832? Of course, this is all assuming that you'd like to see this new API introduced in the 3.3.0 release; if not, then it should be fine to stick with this KIP and keep the discussion here.
On the topic of the builder pattern: 1. I agree that with single-class properties (like key/value (de)serializers) that instances provided to the builder should take priority over any classes specified in the client config. However, with multi-class properties (like interceptors), it may be better to combine the lists in order to accommodate the example provided in the KIP with the SpringAwareProducerConfig. If combination isn't desirable, it's simple enough for developers to nullify the relevant property in the client config. On the other hand, if the client doesn't combine builder-provided and config-provided plugin classes, it'd be trickier to pre-instantiate the ones in the client config, manually combine them with the ones that are already instantiated, and then pass those to the builder. 2. I'd opt to keep the Map/Properties argument in the builder constructor instead of in the build() method, as it's a required parameter and IME those tend to be included in constructors instead of follow-up methods. But it's just a slight preference, both should be fine. 3. Unless we want to make the API and implementation more complex, it'd probably be best to note that any Closeable objects provided to a builder will still be closed once the client is closed, and as a result, it'd be highly recommended to only use a builder once. We might even go so far as to throw an exception if build() is invoked multiple times. 4. We should also decide whether objects that implement the Configurable interface have their configure() method invoked by the builder/client. I suspect we'd want to refrain from invoking it as that would provide more flexibility (you can now customize the instance with a completely separate config from what you give the client) and it would align more closely with the example provided in the KIP, but an argument could be made the other way as it would be slightly more convenient to not have to remember to invoke configure() yourself, and it would probably be less surprising to developers. 5. I see your point about enriching the builder API over adding a general "withConfiguredInstance" method. The reason I originally proposed this is that it'd be easier to maintain in the future and we wouldn't have to remember to expand the builder API whenever adding a new plugin class property to our clients, and the only reason that (de)serializers and interceptors were given special treatment was to address generic type safety holes in the existing API. After sleeping on it, I think I'm convinced that your more-enriched API is better; it's simpler, easier to understand, and addresses non-generics-related type safety holes with the current API (how do I know that the class name for the "partition.assignment.strategy" property actually refers to a class that implements the ConsumerPartitionAssignor interface?). Thumbs up! 6. You mentioned earlier that "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.". As someone who's wrangled with the unused property logging recently [1] I can definitely sympathize with that motivation, although I really wish we could "do things the right way" instead of giving developers a way to work around our well-intentioned-but-not-completely-accurate logic. Overall I think it might be acceptable to address these concerns separately instead of coupling them with this KIP (or a KIP to introduce the builder pattern for clients), but if any of these are pressing for you, we should make note of that and see if there's a way to handle them. 7. Although I'd love it if we could whittle things down to one constructor per client, I think deprecation of the existing constructors is a bit too aggressive of a step for now. We could mention in the KIP that it's an option for the future if this new API is well-accepted, but for now, we'll simply be adding a new constructor and continuing to support all the existing ones as well. Lastly--apologies to the Streams people involved here. I've been saying "client" and "clients" when, most of the time, I should have been appending "and/or Streams" to that as well :) And yes, I definitely do support including KafkaStreams with the builder pattern, although it might get a little tricky given that there's already a widely-used StreamsBuilder [2] class in the Streams API. [1] - https://issues.apache.org/jira/browse/KAFKA-7509 [2] - https://kafka.apache.org/31/javadoc/org/apache/kafka/streams/StreamsBuilder.html Cheers, Chris On Wed, May 11, 2022 at 5:01 AM François Rosière <francois.rosi...@gmail.com> 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 > >> >>>>>>> > >> >>>>>>>>>>>>>>>>>> > >> >>>>>>>>>>>>>>>>>> > >> >>>>>>>>>>>>>>>>> > >> >>>>>>>>>>>>>>>> > >> >>>>>>>>>>>>>>> > >> >>>>>>>>>>>>>> > >> >>>>>>>>>>>>> > >> >>>>>>>>>> > >> >>>>>>>>> > >> >>>>>>> > >> >>>>>> > >> >>>> > >> >>> > >> >> > >> > > >> > > >
