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