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