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