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. 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> > > [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> > > > > 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> > > >>>>>> > > >>>>>>>> > > >>>>>>> > > >>>>>>> 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 > > >>>>>>> > > >>>>>>> > > >>>>>> > > >>>>> > > >>>> > > >> > > > > > > > >
