Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
Hi all, KIP-839 has been created to progress on the builder way https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211884640 Kr, F. Le ven. 13 mai 2022 à 19:15, Colin McCabe a écrit : > I agree. I would really like to see a builder interface here. If you > changed the KIP to add this, you'd have to do the vote again, but I think > it would be well worth it. > > best, > Colin > > > On Thu, May 12, 2022, at 00:13, Bruno Cadonna wrote: > > Hi Francois, > > > > Modifying this KIP or starting a new one is your call. > > > > I guess the idea of the builder might cause some more discussions. So if > > you want to get a working version released as soon as possible, I would > > opt to get the current KIP implemented and create a new one for the > > builder. If time is not an issue I would modify the current KIP. > > > > But as I said: It's your call. > > > > Best, > > Bruno > > > > On 11.05.22 11:01, François Rosière 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() > >>> .withKeyDeserializer() > >>> .withValueDeserializer() > >>> .withInterceptors() > >>> .withMetricsReporter() > >>> .build(); > >>> > >>> new ProducerBuilder() > >>> .withKeySerializer() > >>> .withValueSerializer() > >>> .withInterceptors() > >>> .withPartitioner() > >>> .withMetricsReporter() > >>> .build(); > >>> > >>> new KafkaStreamsBuilder() > >>> .withProducerInterceptors() > >>> .withConsumerInterceptors() > >>> .withTime() > >>> .withKafkaClientSupplier() > >>> .withMetricsReporter() > >>> .build(); > >>> > >>> 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 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 p = new Builder(...) > > .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 expo
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
I agree. I would really like to see a builder interface here. If you changed the KIP to add this, you'd have to do the vote again, but I think it would be well worth it. best, Colin On Thu, May 12, 2022, at 00:13, Bruno Cadonna wrote: > Hi Francois, > > Modifying this KIP or starting a new one is your call. > > I guess the idea of the builder might cause some more discussions. So if > you want to get a working version released as soon as possible, I would > opt to get the current KIP implemented and create a new one for the > builder. If time is not an issue I would modify the current KIP. > > But as I said: It's your call. > > Best, > Bruno > > On 11.05.22 11:01, François Rosière 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 >> 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() >>> .withKeyDeserializer() >>> .withValueDeserializer() >>> .withInterceptors() >>> .withMetricsReporter() >>> .build(); >>> >>> new ProducerBuilder() >>> .withKeySerializer() >>> .withValueSerializer() >>> .withInterceptors() >>> .withPartitioner() >>> .withMetricsReporter() >>> .build(); >>> >>> new KafkaStreamsBuilder() >>> .withProducerInterceptors() >>> .withConsumerInterceptors() >>> .withTime() >>> .withKafkaClientSupplier() >>> .withMetricsReporter() >>> .build(); >>> >>> 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 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 p = new Builder(...) > .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 existi
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
Hi Francois, Modifying this KIP or starting a new one is your call. I guess the idea of the builder might cause some more discussions. So if you want to get a working version released as soon as possible, I would opt to get the current KIP implemented and create a new one for the builder. If time is not an issue I would modify the current KIP. But as I said: It's your call. Best, Bruno On 11.05.22 11:01, François Rosière 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 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() .withKeyDeserializer() .withValueDeserializer() .withInterceptors() .withMetricsReporter() .build(); new ProducerBuilder() .withKeySerializer() .withValueSerializer() .withInterceptors() .withPartitioner() .withMetricsReporter() .build(); new KafkaStreamsBuilder() .withProducerInterceptors() .withConsumerInterceptors() .withTime() .withKafkaClientSupplier() .withMetricsReporter() .build(); 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 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 p = new Builder(...) .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 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
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 pa
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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() >.withKeyDeserializer() >.withValueDeserializer() >.withInterceptors() >.withMetricsReporter() >.build(); > > new ProducerBuilder() >.withKeySerializer() >.withValueSerializer() >.withInterceptors() >.withPartitioner() >.withMetricsReporter() >.build(); > > new KafkaStreamsBuilder() >.withProducerInterceptors() >.withConsumerInterceptors() >.withTime() >.withKafkaClientSupplier() >.withMetricsReporter() >.build(); > > 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 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 p = new Builder(...) >> > .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 a >> >> écrit : >> >> >> >>> Hi Francois, >> >>> >> >>> Thanks for the KIP! I sympathize with the issue you're facing
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
Hello, Builder is clearly the way to go for future releases of Kafka. If we align streams, we would have 3 builders new ConsumerBuilder() .withKeyDeserializer() .withValueDeserializer() .withInterceptors() .withMetricsReporter() .build(); new ProducerBuilder() .withKeySerializer() .withValueSerializer() .withInterceptors() .withPartitioner() .withMetricsReporter() .build(); new KafkaStreamsBuilder() .withProducerInterceptors() .withConsumerInterceptors() .withTime() .withKafkaClientSupplier() .withMetricsReporter() .build(); 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 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 p = new Builder(...) > > .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 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,
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 p = new Builder(...) .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 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 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 is used with a Serializer and Serializer, 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 pa
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 p = new Builder(...) .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 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 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 is used with > a > > Serializer and Serializer, 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 cla
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 is used with a
> Serializer and Serializer, 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 {
> private final Map props;
> private Serializer keySerializer;
> private Serializer valueSerializer;
> private List> interceptors;
> private Map configuredInstances;
> private Map> configuredInstanceLists;
>
> public Builder(Map 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 serializer) {
> this.keySerializer = serializer;
> return this;
> }
>
> public Builder withValueSerializer(Serializer serializer) {
>
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 is used with a
Serializer and Serializer, 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 {
private final Map props;
private Serializer keySerializer;
private Serializer valueSerializer;
private List> interceptors;
private Map configuredInstances;
private Map> configuredInstanceLists;
public Builder(Map 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 serializer) {
this.keySerializer = serializer;
return this;
}
public Builder withValueSerializer(Serializer 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>
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
instances) {
this.configuredInstanceLists.put(property, instances);
return this;
}
public KafkaProducer 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 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 exampl
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 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 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 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 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-contai
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 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 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 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 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 als
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
Can a PR be dropped on Github or do we still need some approval first? Le dim. 8 mai 2022 à 06:08, John Roesler 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 > 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 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 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. > >>>
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 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 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 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 bene
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 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 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 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 shou
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 > 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 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 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? > >> > >>
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 >> 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 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 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 >>
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 > 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 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 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: >>> >
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 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 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 >> >> >> >>> >> >> >> > >> >
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 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 > > > >>> > >> > > >
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 a écrit : Kip link: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 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 > a > > écrit : > > > >> Kip link: > >> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578 > >> > >> > > >
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 a écrit : Kip link: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578
Re: [DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
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 a écrit : > Kip link: > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578 > >
[DISCUSS] KIP-832 Allow creating a producer/consumer using a producer/consumer config
Kip link: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578
[Discuss] KIP-832: Allow creating a producer/consumer using a producer/consumer config
Hi Bruno and frosiere, Thanks for raising this KIP, this would be a useful improvement! I advise just expose Map and Properties to the user. so that the parameters will be consistent with old one. the Advise like : 1, consumer api: public KafkaConsumer(Map configs, Deserializer keyDeserializer, Deserializer valueDeserializer, List> interceptors) 2, producer api: public KafkaProducer(Map configs, Serializer keySerializer, Serializer valueSerializer, List> interceptors) Any idea? Please let me know. Thanks, lqjack
