Hi Sebastien, A constructor for Map and Properties each looks good to me. Thanks for the update to the KIP.
-Bill On Wed, Jun 11, 2025 at 8:16 AM Lucas Brutschy <lbruts...@confluent.io.invalid> wrote: > 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 > > > > >>>>>>> > > > > >>>>>>> > > > > >>>>>> > > > > >>>>> > > > > >>>> > > > > >> > > > > > > > > > > > > > > > > >
