Hi François, Thanks for writing this up! A few thoughts now that I've had some time to think this over:
1. The KIP outlines methods for the KafkaStreamsBuilder class to add producer interceptors and consumer interceptors. Since Streams instantiates many producers and consumers, we should probably leave these methods out of the builder due to the concerns noted in the KIP about shared resources being closed when a client is shut down. The existing KafkaClientSupplier interface should be sufficient to cover the case of interceptors already, but if not, we can explore alternatives that don't risk conflict over shared resources. 2. In a similar vein, the KIP also outlines a method for the KafkaStreamsBuilder class to add metrics reporters. Unlike interceptors, I think there's still some value to this method, since a KafkaStreams instance does instantiate its own set of metrics reporters. However, we should probably be careful to note in the docs for this method (and in the KIP itself) that this method only controls the metrics reporters for Streams-specific metrics, and not for the Kafka clients that Streams uses. We could also point people towards the KafkaClientSupplier interface if they'd like to add metrics reporters for these Kafka clients. 3. We should probably add a builder for the Admin interface as well, since it's also a Kafka client class and comes with the same "metric.reporters" property (which takes a list of classes) that the other two come with. 4. Can you specify the exact package(s) that the new builders will be added to, and the exact type signatures for their constructors and methods? You can see KIP-585 [1] for a good example of how KIPs usually define new classes/interfaces. You don't need to add Javadocs to everything, but if there's something worth mentioning like the behavior proposed in item 2, it's nice to include in a Javadoc for the relevant class/method. 5. Just for aesthetics, I wonder if we can add static builder() methods to the KafkaProducer, KafkaConsumer, KafkaStreams, and Admin classes/interface that return a new builder? Seems cleaner to write "KafkaProducer.builder(props)" than "new KafkaProducerBuilder(props)". Not a huge thing, though. [1] - https://cwiki.apache.org/confluence/display/KAFKA/KIP-585%3A+Filter+and+Conditional+SMTs#KIP585:FilterandConditionalSMTs-PublicInterfaces Cheers, Chris On Thu, May 19, 2022 at 4:59 PM François Rosière <francois.rosi...@gmail.com> wrote: > @Kirk, > > 1. No defaults are expected. Serializers/deserializers would need to either > by provided using the config or the builder. > 2. As some properties would need to be closed, maybe the build method > should only be called one time. To see if we really need to add a check for > that. > > Kr, > > François. > > Le jeu. 19 mai 2022 à 00:02, Kirk True <k...@kirktrue.pro> a écrit : > > > Hi François, > > > > Thanks for the KIP! > > > > A couple of questions: > > > > 1. Do the builders have defaults for the serializers/deserializers? > > 2. Can the build method be called more than once on a given builder? > > > > Thanks, > > Kirk > > > > On Wed, May 18, 2022, at 10:11 AM, François Rosière wrote: > > > Hi all, > > > > > > KIP to create builders for > > > > > > - KafkaProducer > > > - KafkaConsumer > > > - KafkaStreams > > > > > > > > > This KIP can be seen as the continuity of the KIP-832. > > > > > > KIP details: <goog_1519423886> > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211884640 > > > Jira issue: https://issues.apache.org/jira/browse/KAFKA-13913 > > > > > > Kr, > > > > > > F. > > > > > >
