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