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