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