This was nagging at me so I dug around and managed to find the original discussion around needing StreamsConfig for dependency injection: https://github.com/apache/kafka/pull/5344
tl;dr the StreamsConfig overloads aren't doing anything useful on their own, we definitely shouldn't use StreamsConfig for the new constructors in this KIP, but should we go a step further and re-deprecate the KafkaStreams constructors that use StreamsConfig? And do we need a KIP to do so? Slightly longer version: this is all a bit messy but basically this is what went down: 1. That discussion is what resulted in us removing the @Deprecated annotation from the StreamsConfig overloads of the KafkaStreams constructor (without a KIP -- more on this later) 2. the author of that discussion proposed this KIP: KIP-378: Enable Dependency Injection for Kafka Streams handlers <https://cwiki.apache.org/confluence/display/KAFKA/KIP-378%3A+Enable+Dependency+Injection+for+Kafka+Streams+handlers> 3. KIP-378 was abandoned at some point but is apparently subsumed by KIP-832: Allow creating a producer/consumer using a producer/consumer config <https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=211882578> 4. KIP-832 in turn was discarded in favor of yet another KIP, KIP-839: Provide builders for KafkaProducer/KafkaConsumer and KafkaStreams <https://cwiki.apache.org/confluence/x/YBqhD> Ultimately it seems like none of these KIPs were accepted or implemented so the problem is still there. I'm also personally not sure why/how KIP-832 subsumes KIP-378 as it seems to address only the client supplier part of the problem, whereas my understanding is that all statically created/configured StreamsConfig objects would be affected (eg DeserializationExceptionHandler) Anyways, whatever the specifics, all of these KIPs were discarded at some point. And IIUC the StreamsConfig overload is a necessary but sufficient condition here -- that is, they needed the StreamsConfig overloads as part of the original fix, but also had to make some further changes in order to enable proper dependency injection in Spring like they wanted. So just having a constructor that accepts StreamsConfig isn't doing anything without the rest of the fix (ie KIP-378) and therefore it doesn't really make sense to offer a StreamsConfig overload unless/until we have the full set of changes. So this definitely seems to answer the question for the StreamsBuilder/Topology constructors, that is, no need for a StreamsConfig version of these APIs. Which is enough to proceed with this KIP. But it does kind of raise another question, which is, should we just re-deprecate the StreamsConfig overloads of KafkaStreams? It seems like when (or more likely, if) the dependency injection stuff is picked up and finished then we can add these overloads back in as part of that KIP. Until then, they're just increasing the surface area of the already bloated KafkaStreams constructor. Assuming we do want to re-deprecate these, would we need a KIP? There was already a KIP to deprecate them in the first place of course (KIP-245 <https://cwiki.apache.org/confluence/display/KAFKA/KIP-245%3A+Use+Properties+instead+of+StreamsConfig+in+KafkaStreams+constructor>) but what's interesting is that it seems we never did a KIP to *remove* the deprecation. Seems like we were evaluating whether or not to remove these KafkaStreams constructors during a major release and decided against doing so due to the concerns raised in the discussion I linked to. And I guess we decided that if we weren't going to remove them then we should just go ahead and un-deprecate them...or something like that (#10484 <https://github.com/apache/kafka/pull/10484>). So...if there was no KIP to un-deprecate them, can we re-deprecate them without a KIP? On Wed, Apr 2, 2025 at 10:02 PM Sophie Blee-Goldman <sop...@responsive.dev> wrote: > 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? > > 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 >> > [2] >> > >> > >> 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 >> > >>>>>> >> > >>>>>>>> >> > >>>>>>> >> > >>>>>>> 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 >> > >>>>>>> >> > >>>>>>> >> > >>>>>> >> > >>>>> >> > >>>> >> > >> >> > > >> > >> > >> >
