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 >>> >>>> >>>> >>> >>>> >>>> >>> >>>> >>> >>> >>>> >> >>> >>>> > >>> >>>> >>> >>> >>> >>
