Hi Sebastien, having both Map/Properties sounds good to me.
In the example in under `New way to pass topology-specific config:`, you still seem to use the `StreamConfig` constructor. Could you update the example as well? Cheers, Lucas On Fri, May 23, 2025 at 3:32 PM Sebastien Viale <sebastien.vi...@michelin.com> wrote: > > Hi all, > > I’d like to reopen the discussion (I’ve been really busy over the past two > months) > > I’ve updated the KIP to include two constructors: one accepting a Map and one > accepting Properties. > > Does that look ok for everyone ? > > Thanks > > Sébastien > > > De : Sebastien Viale <sebastien.vi...@michelin.com> > Date : jeudi, 3 avril 2025 à 14:11 > À : dev <dev@kafka.apache.org> > Objet : Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for supplying > configs needed by the topology > Thanks, everyone, and thanks, Sophie, for this great summary. > Indeed, the KafkaStreams constructor uses a Properties object, so it might be > more concise to use it. > Even if it is overkill, some existing applications may already use a > Properties, while others use a Map. Personally, I think it is reasonable to > propose both constructors. However, I wouldn’t be upset if we kept only one > of them. > I will wait for final thoughts before updating the KIP with the new > constructors. > Cheers ! > > Sébastien > > De : Sophie Blee-Goldman <sop...@responsive.dev> > Date : jeudi, 3 avril 2025 à 11:33 > À : dev@kafka.apache.org <dev@kafka.apache.org> > Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and API for > supplying configs needed by the topology > Warning External sender Do not click on any links or open any attachments > unless you trust the sender and know the content is safe. > > Sounds like we are in agreement and can move forward with this KIP. > > To sum, instead of having the new StreamsBuilder and Topology constructors > take a StreamsConfig parameter, they should take a Map or Properties > instead. > > Between the two of those I don't personally have much of a preference, and > some clients (like KafkaConsumer) even seem to offer both a Map and a > Properties version of the constructor. Imho this is a bit of an overkill > and choosing one or the other should be fine for us. I'll note that the the > KafkaStreams constructor uses Properties so perhaps that's the appropriate > choice here (even if I personally like the plain Map option slightly better) > > Any final thoughts? > This email was screened for spam and malicious content but exercise caution > anyway. > This email was screened for spam and malicious content but exercise caution > anyway. > > > > > > On Thu, Mar 27, 2025 at 9:17 AM Bill Bejeck <bbej...@gmail.com> wrote: > > > Hi All, > > > > I also agree with the having consistency with the other clients, so I'm > > also a +1 for using Properties/Map. > > > > Thanks, > > Bill > > > > On Thu, Mar 27, 2025 at 7:28 AM Bruno Cadonna <cado...@apache.org> wrote: > > > > > Hi, > > > > > > I proposed to use constructors with StreamsConfig because I wanted a > > > public API that says to the users: "The config you pass to Streams will > > > not be modified." That the first call in a constructor is a > > > transformation from Properties/Map to StreamsConfig a user might or > > > might not know. > > > > > > I looked a bit around and found KIP-245[1] that deprecated KafkaStreams > > > constructors that take a StreamsConfig. The main motivation was that > > > those constructors add boilerplate because you need to construct an > > > additional object. Fair enough! > > > > > > Apparently, we also allow to modify the config in constructors[2]. > > > > > > So the immutability property is not even possible without making larger > > > changes. > > > > > > I agree that we should keep it consistent with the other clients and I > > > also see the simplicity argument. So I am fine with just using > > > Properties/Map. > > > > > > Would be interesting to understand what the problem with the dependency > > > injection is. In the end a StreamsConfig is constructed from a Map. > > > > > > > > > Best, > > > Bruno > > > > > > > > > [1] > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor><https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor<https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor>> > > > [2] > > > > > > > > https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600><https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600<https://github.com/apache/kafka/blob/b9d5597b445741330bf561b096c25e28224988eb/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L600>> > > > > > > On 26.03.25 22:33, Sophie Blee-Goldman wrote: > > > > I think Lucas makes a good point. If we want to maintain consistency > > with > > > > the currents state and intended direction of the clients w.r.t config > > > > classes, we should definitely (and perhaps only) offer the > > Map/Properties > > > > version of the APIs > > > > > > > > I even submitted a KIP a while back to open up the visibility of one of > > > the > > > > ProducerConfig constructors, just to bring it in line with the other > > > > config classes (and to help shut off excessive logging), and it was > > > pretty > > > > much stopped dead by core/clients people who didn't like the current > > > > openness of these config classes and wanted to move in the opposite > > > > direction rather than make anything more visible (Even if just to bring > > > one > > > > of them to parity with the other classes) > > > > > > > > I think this is a pretty good argument for at the very least offering > > the > > > > plain Map/Properties version, and perhaps only offering that. IIRC the > > > only > > > > reason we added the StreamsConfig version of the KafkaStreams > > > constructors > > > > was to enable dependency injection for spring kafka users. I have to > > > wonder > > > > if there is a way to fix this in spring kafka to allow Map/Properties > > > based > > > > configuration, rather than forcing KafkaStreams to allow StreamsConfig > > > > constructors > > > > > > > > So as of this time I'm still personally in favor of having only the > > > > Map/Properties option, though that still leaves open the question of > > > which > > > > one (I notice that KAfkaConsumer, for example, offers both -- but this > > > > seems unnecessary and I think either Map or Properties, but not both, > > is > > > > sufficient) > > > > > > > > All this said I don't want to block the KIP forever on this question, > > so > > > I > > > > think we should try to wrap things up one way or the other. If we can't > > > > come to a consensus I'd advocate to just offer both a Map/Properties > > and > > > a > > > > StreamsConfig version. That way if/when we ever have to deprecate one > > of > > > > them, it's less disruptive to everyone who hopefully was already using > > > the > > > > other option (imo it's more likely we'd want/need to deprecate the > > > > StreamsConfig version and I also feel this will be the less popular > > > option > > > > since it seems only needed by those using spring) > > > > > > > > Thoughts? > > > > > > > > On Mon, Mar 24, 2025 at 2:11 PM Lucas Brutschy > > > > <lbruts...@confluent.io.invalid> wrote: > > > > > > > >> Hi, > > > >> > > > >> I agree with the KIP, it makes sense to reduce the number of ways the > > > >> configs get passed. I agree with Sophie that it doesn't make sense to > > > >> rewrite the topology compilation just to delay reading the configs. > > > >> > > > >> The question StreamsConfig vs. Map/Properties is less clear-cut to me. > > > >> The immutability argument seems weak - if a parameter of type > > > >> Properties / Map is converted into a StreamsConfig copy right in the > > > >> various constructors, we'd get the same immutability benefit. > > > >> > > > >> On the other hand, KafkaConsumer, KafkaProducer, AdminClient all take > > > >> Map<String, Object> or Properties, and explicitly restrict the > > > >> visibility of constructors with ConsumerConfig/ProducerConfig/..., and > > > >> KafkaStreams would decide to go a separate route, allowing only the > > > >> complex AbstractConfig class instead of the simple data type. This > > > >> seems pretty inconsistent for the interfaces of a single open source > > > >> project. > > > >> > > > >> Cheers, > > > >> Lucas > > > >> > > > >> On Fri, Mar 14, 2025 at 2:43 AM Sophie Blee-Goldman > > > >> <sop...@responsive.dev> wrote: > > > >>> > > > >>> I don't believe it's possible to only pass the configurations into > > > >>> KafkaStreams, at least not without a huge refactoring of how and when > > > the > > > >>> topology is compiled. I looked into this for the processor wrapper > > > config > > > >>> since obviously it's preferable to not require configs be passed in > > to > > > >> the > > > >>> Topology/StreamsBuilder, and wasn't able to make it work. If it's > > > >> possible > > > >>> at all, it would have significantly complicated the already-complex > > > >>> topology construction code. I don't think it's worth it. > > > >>> > > > >>> If we really wanted to only pass configs into one place, I'd say we > > > >> should > > > >>> just require them for the Topology and StreeamsBuilder constructor, > > and > > > >> not > > > >>> pass configs into KafkaStreams at all. But that would mean a > > > >>> deprecation/code change affecting every single user out there, which > > > >> feels > > > >>> like unnecessary turmoil. > > > >>> > > > >>> As for whether to require StreamsConfig vs Map/Properties, I'm fine > > > with > > > >>> requiring StreamsConfig in theory, but since KafkaStreams allows > > users > > > to > > > >>> pass in a plain config map then some users might not instantiate > > > >>> StreamsConfig at all, and this would force them to do so. Maybe > > that's > > > >> not > > > >>> such a bad thing though...so yeah, guess I'm fine with just the > > > >>> StreamsConfig variant :) > > > >>> > > > >>> On Thu, Mar 13, 2025 at 3:17 PM Bill Bejeck <bbej...@apache.org> > > > wrote: > > > >>> > > > >>>> Thanks for the KIP, this is great improvement. > > > >>>> > > > >>>> I'm +1 to all the comments and changes in the KIP thus far. > > > >>>> > > > >>>> I also like the proposal from Bruno regarding only passing > > > >> configurations > > > >>>> to KafkaStreams. > > > >>>> Regarding your comment about internal configuration propagation, > > would > > > >>>> setting the visiblitly on the Topology constructor to package > > private > > > >> or > > > >>>> protected solve the issue? > > > >>>> I'm not sure I'm addressing your concern entirely, but if we can > > > >> simplify > > > >>>> the code paths for passing configuration, I think it's worth some > > > >> tradeoffs > > > >>>> for the gain in simplicity. > > > >>>> As I'm writing this though I'm wondering if this is indeed possible > > > >> when > > > >>>> taking the PAPI into consideration. At any rate, I think the KIP > > can > > > >>>> proceed as is, I just wanted to add my 2cents. > > > >>>> > > > >>>> Regards, > > > >>>> Bill > > > >>>> > > > >>>> > > > >>>> On Mon, Mar 10, 2025 at 11:38 AM Sebastien Viale < > > > >>>> sebastien.vi...@michelin.com> wrote: > > > >>>> > > > >>>>> Hi, > > > >>>>> > > > >>>>> Thank you both for your remarks. > > > >>>>> > > > >>>>> Sophie: > > > >>>>> > > > >>>>> I have added all your proposals to the rejected alternatives. > > > >>>>> > > > >>>>> BC1: > > > >>>>> I agree that we should not create multiple config classes, as > > > >> avoiding > > > >>>>> that is the main goal of the KIP. > > > >>>>> > > > >>>>> It would be nice to have a single constructor (KafkaStreams) that > > > >> accepts > > > >>>>> the configuration. However, unless I’m mistaken, this would require > > > >>>>> handling config propagation internally to other components like > > > >> Topology. > > > >>>>> > > > >>>>> BC2: > > > >>>>> StreamsConfig is indeed immutable, and like Bruno, I believe this > > is > > > >> the > > > >>>>> safer approach. > > > >>>>> Best, > > > >>>>> Sébastien > > > >>>>> > > > >>>>> > > > >>>>> De : Bruno Cadonna <cado...@apache.org> > > > >>>>> Date : vendredi, 7 mars 2025 à 18:55 > > > >>>>> À : dev@kafka.apache.org <dev@kafka.apache.org> > > > >>>>> Objet : [EXT] Re: [DISCUSS] KIP-1138: Clean up TopologyConfig and > > > >> API for > > > >>>>> supplying configs needed by the topology > > > >>>>> Warning External sender Do not click on any links or open any > > > >> attachments > > > >>>>> unless you trust the sender and know the content is safe. > > > >>>>> > > > >>>>> Hi Sébastien, > > > >>>>> > > > >>>>> Thanks for the KIP! > > > >>>>> > > > >>>>> Cleaning up the configs classes is really a good idea! > > > >>>>> > > > >>>>> BC1: > > > >>>>> I am in favor of having only StreamsConfig as a config class to > > avoid > > > >>>>> proliferation of config classes. However, I find it a bit ugly > > that a > > > >>>>> Streams user is required to pass the config first to the topology > > and > > > >>>>> then to KafkaStreams. Since we are talking about changing > > > >> constructors, > > > >>>>> I am wondering if it were possible to pass the configs only to > > > >>>>> KafkaStreams. > > > >>>>> > > > >>>>> BC2: > > > >>>>> While Sophie advocates to only pass a Map/Properties object into > > the > > > >>>>> constructors, I am advocating to only passing StreamsConfig objects > > > >> to > > > >>>>> the constructors. The reason is that Map/Properties are mutable and > > > >>>>> StreamsConfig is immutable. IMO, it would be safer to pass > > immutable > > > >>>>> configs to the constructors. But maybe there is a good reason to > > pass > > > >>>>> mutable configs that I do not know. Maybe Sophie can clarify. > > > >>>>> > > > >>>>> +1 to adding all considerations discussed in the thread to the > > > >> rejected > > > >>>>> alternatives. > > > >>>>> > > > >>>>> > > > >>>>> Best, > > > >>>>> Bruno > > > >>>>> This email was screened for spam and malicious content but exercise > > > >>>>> caution anyway. > > > >>>>> > > > >>>>> > > > >>>>> > > > >>>>> > > > >>>>> > > > >>>>> On 06.03.25 02:43, Sophie Blee-Goldman wrote: > > > >>>>>> Thanks for the KIP! The proposal here sounds like the right > > > >> direction > > > >>>> to > > > >>>>> go > > > >>>>>> in for me, but I'll be interested to hear what others think. > > > >>>>>> > > > >>>>>> I'm wondering if we should offer additional StreamsBuilder & > > > >> Topology > > > >>>>>> constructors that accept a plain config map (or Properties) > > > >> alongside > > > >>>> the > > > >>>>>> StreamsConfig constructors. I'd advocate for providing only the > > > >>>>>> Map/Properties variety except that I'm pretty sure the > > > >> StreamsConfig > > > >>>>>> options are needed for dependency injection and were requested by > > > >> users > > > >>>>> of > > > >>>>>> Spring > > > >>>>>> > > > >>>>>> Also, just to list out some alternatives, we could go in a > > > >> different > > > >>>>>> direction completely by making TopologyConfig a true config in its > > > >> own > > > >>>>>> right, and moving all the topology-specific configs from > > > >> StreamsConfig > > > >>>> to > > > >>>>>> TopologyConfig (ie removing them from StreamsConfig completely) > > > >>>>>> > > > >>>>>> On the one hand, it might make more sense to keep configs > > > >> separated and > > > >>>>>> group them by where they need to be applied. On the other hand I > > > >>>> wouldn't > > > >>>>>> want to start proliferating config classes since I think this > > makes > > > >>>>>> discovery more difficult and increases the API surface area. So > > > >>>>> personally, > > > >>>>>> I think it's sufficient to just add the check and warning log > > > >> described > > > >>>>> in > > > >>>>>> the KIP > > > >>>>>> > > > >>>>>> We could also deprecate the default constructor for > > > >>>>> TopologyStreamsBuilder > > > >>>>>> to force people to pass configs in. I'm honestly fine either way, > > > >> as I > > > >>>>>> don't really think it's much of a burden to ask users to pass > > > >> configs > > > >>>>> into > > > >>>>>> one more place, but since the new check should help prevent users > > > >> from > > > >>>>>> accidentally forgetting to pass the topology-specific configs into > > > >> the > > > >>>>>> Topology/StreamsBuilder, then deprecating the default constructors > > > >> seem > > > >>>>>> like na unnecessary step. > > > >>>>>> > > > >>>>>> Sébastien -- My only other feedback for you on the KIP would be to > > > >> list > > > >>>>>> these under the Rejected Alternatives section so we know what the > > > >> range > > > >>>>> of > > > >>>>>> options are. > > > >>>>>> > > > >>>>>> Thanks! > > > >>>>>> -Sophie > > > >>>>>> > > > >>>>>> On Wed, Feb 26, 2025 at 11:57 PM Sebastien Viale < > > > >>>>>> sebastien.vi...@michelin.com> wrote: > > > >>>>>> > > > >>>>>>> Hi Everyone, > > > >>>>>>> > > > >>>>>>> I would like to start a discussion on KIP-1138: Clean up > > > >>>> TopologyConfig > > > >>>>>>> and API for supplying configs needed by the topology< > > > >>>>>>> > > > >>>>> > > > >>>> > > > >> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>> > > > >>>>> < > > > >>>>> > > > >>>> > > > >> > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology><https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology<https://cwiki.apache.org/confluence/display/KAFKA/KIP-1138%3A+Clean+up+TopologyConfig+and+API+for+supplying+configs+needed+by+the+topology>> > > > >>>>>> > > > >>>>>>>> > > > >>>>>>> > > > >>>>>>> This proposal aims to simplify Kafka Streams configuration by > > > >> unifying > > > >>>>>>> APIs and ensuring topology-specific configs (e.g., > > > >>>>> topology.optimization, > > > >>>>>>> processor.wrapper.class) are correctly applied to prevent silent > > > >>>>>>> misconfigurations. > > > >>>>>>> > > > >>>>>>> Regards, > > > >>>>>>> > > > >>>>>>> Sébastien > > > >>>>>>> > > > >>>>>>> > > > >>>>>> > > > >>>>> > > > >>>> > > > >> > > > > > > > > > > > >
