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